MetaMask / metamask-mobile

Mobile web browser providing access to websites that use the Ethereum blockchain
https://metamask.io
Other
2.04k stars 1.06k forks source link

feat: edit networks UI redesign #10040

Closed EtherWizard33 closed 5 days ago

EtherWizard33 commented 1 week ago

Description

This PR adds the ability to edit networks. This PR also includes Salim's work from the feat-edit-delete-network-menu branch.

For a short demo please view this loom: https://www.loom.com/share/1192d8d7846845e69da9894def13aeeb

Related issues

Fixes:

Manual testing steps

  1. Click the network selector at the center top of the screen, a bottom sheet comes up with a list of networks
  2. On the right side of enabled networks, a 3 dot icon is displayed as a 'more' menu, tap it to edit the network
  3. The edit form shows up pre-filled with the network details to be edited

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions[bot] commented 1 week ago

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

EtherWizard33 commented 6 days ago

can you provide a few screenshots/videos of the new components?

Hi Brian, yes here is a gif and screenshot, let me know if that works, there is also a link to a loom video in the description in case that is also of interest.

gif screenshot
Screenshot 2024-04-18 at 3 56 43 PM Screenshot 2024-04-18 at 3 56 43 PM
EtherWizard33 commented 6 days ago

storybook/storyLoader.js

@brianacnguyen also here is a screenshot from storybook

cell-with-three-dots
github-actions[bot] commented 6 days ago

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 7e8ec14fb4a6a0f2ec68e84f5943c3f7efe03ef4 Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/6f441944-c403-47c6-a762-685b7ac81653

[!NOTE]

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request
github-actions[bot] commented 6 days ago

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: bfd71adbdc129e08a476f5fbd1e0e98b09a59c1b Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/8d333064-eda4-403f-b753-455e804d139c

[!NOTE]

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request
github-actions[bot] commented 5 days ago

https://bitrise.io/ Bitrise

🔄🔄🔄 pr_smoke_e2e_pipeline started on Bitrise...🔄🔄🔄

Commit hash: 7325da2c8f0e1b6a08e568412c09dc966e1cb73a Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/4a9a0b39-b3f4-4e0d-8367-9bcc1017903f

[!NOTE]

  • This comment will auto-update when build completes
  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request
github-actions[bot] commented 5 days ago

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 7249902620a40a111ae57f146e3badf7bed52149 Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/9042aef4-7f9d-441f-abf2-fc5dcd62e60a

[!NOTE]

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request
github-actions[bot] commented 5 days ago

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: a943a47a41f3c45ec120a4457d4894ab15e73ca9 Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/f16ba45f-5dee-4f6b-9be5-efb2c592b907

[!NOTE]

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request
sonarcloud[bot] commented 5 days ago

Quality Gate Passed Quality Gate passed

Issues
5 New issues
0 Accepted issues

Measures
0 Security Hotspots
49.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

brianacnguyen commented 5 days ago

This was merged before I could approve. Sorry about the review delay, but in the future can you wait until the approval to merge? There were a few changes I was gonna request Specifically:

EtherWizard33 commented 1 day ago

This was merged before I could approve. Sorry about the review delay, but in the future can you wait until the approval to merge? There were a few changes I was gonna request Specifically:

  • CellSelectWithMenu should be named CellMultiSelectWithMenu since Select indicates single select
  • ListItemMultiSelectButton is not needed. CellMultiSelectWithMenu can be built off of CellMultiSelect since CellMultiSelect is built off of CellBase and ListItemMultiSelect, which is what youre doing Can you make this update in a subsequent PR?

Hey @brianacnguyen,

Hope you're doing great! Big thanks for your sharp eye and feedback—it really means a lot. My bad for the quick merge; it was a bit of a mix-up with a piece from Salim's PR.

Good news, though! Salim's on board to tweak things as you suggested in a new PR. He'll check in with you to make sure it's all good.

If there's anything else you need or something's on your mind, just give me a shout. Really value your thoughts and happy to help out anytime.