Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: updated edit modals #27623

Merged
merged 5 commits into from
Oct 16, 2024
Merged

fix: updated edit modals #27623

merged 5 commits into from
Oct 16, 2024

Conversation

NidhiKJha
Copy link
Member

@NidhiKJha NidhiKJha commented Oct 4, 2024

This PR is to update edit modals followed by design QA

Description

Following changes have been made in this PR:

  1. The update CTA is now fixed/pinned to the bottom

  2. The warning message while clicking on disconnect has been updated:

  • Simple the copy to This will disconnect you from this site
  • Increase Icon size from 12px to 16px
  • Center align warning message with the button

NOTE: Add new accounts will be handled in a separate PR
Including RPC URL is a new change proposed so I will update that in a different PR as well

Related issues

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

Manual testing steps

  1. Run extension with yarn start
  2. Connect to dapp
  3. Click on All Permissions and the go to individual permission page
  4. Click on Edit Accounts Modal and observe the above UI changes

Screenshots/Recordings

Before

Screen.Recording.2024-10-04.at.1.25.19.PM.mov

After

Screen.Recording.2024-10-04.at.1.22.12.PM.mov

Pre-merge author checklist

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.

@NidhiKJha NidhiKJha requested review from a team as code owners October 4, 2024 12:25
@NidhiKJha NidhiKJha added team-wallet-ux needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. labels Oct 4, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [dad8ffe]
Page Load Metrics (1746 ± 88 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint42123761676342164
domContentLoaded14632169170814469
load14702383174618288
domInteractive1580392010
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: -305 Bytes (-0.00%)
  • common: -34 Bytes (-0.00%)

andreahaku
andreahaku previously approved these changes Oct 9, 2024
Copy link

@andreahaku andreahaku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

sahar-fehri
sahar-fehri previously approved these changes Oct 11, 2024
@NidhiKJha NidhiKJha added this pull request to the merge queue Oct 11, 2024
github-merge-queue bot pushed a commit that referenced this pull request Oct 11, 2024
This PR is to update edit modals followed by design QA

## **Description**

Following changes have been made in this PR:

1. The update CTA is now fixed/pinned to the bottom

2. The warning message while clicking on disconnect has been updated:

- Simple the copy to This will disconnect you from this site
- Increase Icon size from 12px to 16px
- Center align warning message with the button

NOTE: Add new accounts will be handled in a separate PR
Including RPC URL is a new change proposed so I will update that in a
different PR as well

## **Related issues**

Fixes:
[https://github.com/MetaMask/MetaMask-planning/issues/3390](https://github.com/MetaMask/MetaMask-planning/issues/3390)

## **Manual testing steps**

1. Run extension with yarn start
2. Connect to dapp
3. Click on All Permissions and the go to individual permission page
4. Click on Edit Accounts Modal and observe the above UI changes

## **Screenshots/Recordings**


### **Before**


https://github.com/user-attachments/assets/8d3c95e8-5f49-486d-9d70-69e50339fe43



### **After**


https://github.com/user-attachments/assets/9f48b909-8c3a-4435-9a38-99b71e05e5d2



## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] 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](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
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.
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 11, 2024
salimtb
salimtb previously approved these changes Oct 11, 2024
@NidhiKJha NidhiKJha added this pull request to the merge queue Oct 14, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 14, 2024
@NidhiKJha NidhiKJha enabled auto-merge October 14, 2024 12:41
@NidhiKJha NidhiKJha added this pull request to the merge queue Oct 14, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [522be69]
Page Load Metrics (2050 ± 161 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint25628801811681327
domContentLoaded168726001958226109
load170230412050336161
domInteractive20158503115
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: -305 Bytes (-0.00%)
  • common: -34 Bytes (-0.00%)

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 14, 2024
@NidhiKJha NidhiKJha added this pull request to the merge queue Oct 14, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 14, 2024
@NidhiKJha NidhiKJha added this pull request to the merge queue Oct 14, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Oct 14, 2024
@NidhiKJha NidhiKJha dismissed stale reviews from salimtb, sahar-fehri, and andreahaku via 59bc530 October 14, 2024 17:16
@metamaskbot
Copy link
Collaborator

Builds ready [59bc530]
Page Load Metrics (1887 ± 108 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint40524151823390187
domContentLoaded160523901851219105
load161624331887226108
domInteractive26180503316
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: 500 Bytes (0.01%)
  • common: -34 Bytes (-0.00%)

Copy link

@andreahaku andreahaku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small comment on a label value

@metamaskbot
Copy link
Collaborator

Builds ready [073fdc1]
Page Load Metrics (2009 ± 98 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint17702510201119995
domContentLoaded17462441197220197
load17722512200920398
domInteractive21110502512
backgroundConnect8107372914
firstReactRender621621022613
getState46520199
initialActions01000
loadScripts12511929149018187
setupStore126926199
uiStartup197828012228229110
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: -286 Bytes (-0.00%)
  • common: -34 Bytes (-0.00%)

jonybur
jonybur previously approved these changes Oct 16, 2024
andreahaku
andreahaku previously approved these changes Oct 16, 2024
Copy link

@andreahaku andreahaku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@NidhiKJha NidhiKJha added this pull request to the merge queue Oct 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 16, 2024
@NidhiKJha NidhiKJha added this pull request to the merge queue Oct 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Oct 16, 2024
sahar-fehri
sahar-fehri previously approved these changes Oct 16, 2024
andreahaku
andreahaku previously approved these changes Oct 16, 2024
Copy link

@andreahaku andreahaku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@NidhiKJha NidhiKJha dismissed stale reviews from andreahaku and sahar-fehri via 24d9f3c October 16, 2024 15:44
Copy link

@metamaskbot
Copy link
Collaborator

Builds ready [24d9f3c]
Page Load Metrics (1771 ± 92 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint25522441701387186
domContentLoaded14522190174018287
load14602199177119392
domInteractive16233494622
backgroundConnect893392612
firstReactRender44194873919
getState565332211
initialActions01000
loadScripts10231777128015976
setupStore116624168
uiStartup17072384198718790
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: -286 Bytes (-0.00%)
  • common: -34 Bytes (-0.00%)

@darkwing
Copy link
Contributor

Approved; useful follow up would be to make the "Select all" and "New Account" static as well, and just have the accounts list scroll.

@NidhiKJha NidhiKJha added this pull request to the merge queue Oct 16, 2024
Merged via the queue into develop with commit 526d3ec Oct 16, 2024
78 checks passed
@NidhiKJha NidhiKJha deleted the fix-3390 branch October 16, 2024 17:03
@github-actions github-actions bot locked and limited conversation to collaborators Oct 16, 2024
@metamaskbot metamaskbot added the release-12.7.0 Issue or pull request that will be included in release 12.7.0 label Oct 16, 2024
@gauthierpetetin gauthierpetetin added release-12.6.0 Issue or pull request that will be included in release 12.6.0 and removed release-12.7.0 Issue or pull request that will be included in release 12.7.0 labels Oct 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. release-12.6.0 Issue or pull request that will be included in release 12.6.0 team-wallet-ux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants