feat: added edit accounts modal by NidhiKJha · Pull Request #26413 · MetaMask/metamask-extension

NidhiKJha · 2024-08-14T10:08:14Z

This PR is to add an edit accounts modal

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2686

Manual testing steps

Go to EditAccountsModal in storybook
Note: Two AvatarAccounts in This modal is not a bug related to this PR

Screenshots/Recordings

Before

NA

After

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

metamaskbot · 2024-08-27T15:46:23Z

Builds ready [2f04498]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (93 ± 22 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	75	295	127	54	26
		domContentLoaded	42	209	89	46	22
		load	48	216	93	45	22
		domInteractive	10	147	33	31	15

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 0 Bytes (0.00%)
ui: 3.55 KiB (0.05%)
common: 39 Bytes (0.00%)

codecov · 2024-08-27T15:47:20Z

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Project coverage is 70.12%. Comparing base (761562a) to head (78b7533).
Report is 28 commits behind head on develop.

Files with missing lines	Patch %	Lines
...ichain/edit-accounts-modal/edit-accounts-modal.tsx	84.62%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           develop   #26413   +/-   ##
========================================
  Coverage    70.11%   70.12%           
========================================
  Files         1416     1417    +1     
  Lines        49389    49402   +13     
  Branches     13799    13800    +1     
========================================
+ Hits         34628    34639   +11     
- Misses       14761    14763    +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

salimtb · 2024-08-27T16:50:20Z

ui/components/multichain/edit-accounts-modal/edit-accounts-modal.test.tsx

+  return renderWithProvider(<EditAccountsModal {...props} />, store);
+};
+describe('EditAccountsModal', () => {
+  it('should render correctly', () => {


can we add more unit test for this component:

calls onClose when the close button is clicked

handles checkbox click

+1. Also if possible, a test for the Select All button

Checkbox click is for account list item component and it's included there and so is the onClose for modal but I added a test for select all

jonybur

Good start! Please check out my comments

ui/components/multichain/edit-accounts-modal/edit-accounts-modal.tsx

jonybur · 2024-08-28T00:48:11Z

ui/components/multichain/edit-accounts-modal/edit-accounts-modal.tsx

+            isPinned={Boolean(account.pinned)}
+            startAccessory={<Checkbox isChecked />}
+            onClick={() => console.log(null)}
+            selected={false}


Please finish implementing this

nit: might be nice to consider including the avatar with badge for the account, as well as the tokens avatargroup, with mock values, that would get the UI done to what the corresponding issue shows.

Doing this was useful for mobile since it revealed some adjustments DS team had to do to these components who had not been used much yet on mobile, maybe your extension components are already matching figma

It's a dumb component so having a console on click is fine. The selection is not gonna be based on this component so it's fine to keep it this way. @jonybur

@EtherWizard33 the connected badges for avatar is visible when it's in the popup view. So, it's already included in there

jonybur · 2024-08-28T00:48:30Z

ui/components/multichain/edit-accounts-modal/edit-accounts-modal.tsx

+            // isIndeterminate={isIndeterminate}
+          />
+        </Box>
+        {mergedAccounts.map((account) => (


Also add the key to the map

It does have a key

True, sorry

jonybur · 2024-08-28T00:49:18Z

ui/components/multichain/edit-accounts-modal/edit-accounts-modal.tsx

+
+type EditAccountsModalProps = {
+  onClose: () => void;
+  allowedAccountTypes?: KeyringAccountType[]; // Made optional to match default


To match what default?

sonarqubecloud · 2024-08-29T20:47:57Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

metamaskbot · 2024-08-29T21:16:06Z

Builds ready [78b7533]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1671 ± 71 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1501	2242	1668	146	70
		domContentLoaded	1493	2106	1644	122	58
		load	1501	2253	1671	148	71
		domInteractive	22	65	35	15	7

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 0 Bytes (0.00%)
ui: 3.55 KiB (0.05%)
common: 39 Bytes (0.00%)

metamaskbot added the team-core-extension-ux Core Extension UX team label Aug 14, 2024

NidhiKJha marked this pull request as ready for review August 27, 2024 13:12

NidhiKJha requested a review from a team as a code owner August 27, 2024 13:12

NidhiKJha added the needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. label Aug 27, 2024

NidhiKJha requested a review from jonybur August 27, 2024 13:12

metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Aug 27, 2024

salimtb reviewed Aug 27, 2024

View reviewed changes

jonybur suggested changes Aug 28, 2024

View reviewed changes

NidhiKJha added 3 commits August 29, 2024 14:21

added edit accounts modal

04faa9c

updated lint errors

ad91b8a

added test for select all

d6485ce

NidhiKJha force-pushed the edit-accounts-modal branch from 2f04498 to d6485ce Compare August 29, 2024 13:21

hesterbruikman added the design-qa label Aug 29, 2024

NidhiKJha and others added 2 commits August 29, 2024 16:49

lint fix

c0b70ee

Merge branch 'develop' into edit-accounts-modal

78b7533

darkwing approved these changes Aug 30, 2024

View reviewed changes

jonybur approved these changes Sep 2, 2024

View reviewed changes

NidhiKJha merged commit 1ef0c49 into develop Sep 2, 2024

NidhiKJha deleted the edit-accounts-modal branch September 2, 2024 21:20

github-actions bot locked and limited conversation to collaborators Sep 2, 2024

metamaskbot added the release-12.6.0 Issue or pull request that will be included in release 12.6.0 label Sep 2, 2024

gauthierpetetin added release-12.5.0 Issue or pull request that will be included in release 12.5.0 and removed release-12.6.0 Issue or pull request that will be included in release 12.6.0 labels Sep 11, 2024

Conversation

NidhiKJha commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

metamaskbot commented Aug 27, 2024

Uh oh!

codecov bot commented Aug 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonybur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EtherWizard33 Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Aug 29, 2024

Quality Gate passed

Uh oh!

metamaskbot commented Aug 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

NidhiKJha commented Aug 14, 2024 •

edited

Loading

codecov bot commented Aug 27, 2024 •

edited

Loading

EtherWizard33 Aug 28, 2024 •

edited

Loading