Skip to content

refactor(snap-keyring): refactor confirmation dialogs#23007

Merged
ccharly merged 14 commits intodevelopfrom
refactor/snap-keyring-ui-messages
Mar 1, 2024
Merged

refactor(snap-keyring): refactor confirmation dialogs#23007
ccharly merged 14 commits intodevelopfrom
refactor/snap-keyring-ui-messages

Conversation

@ccharly
Copy link
Copy Markdown
Contributor

@ccharly ccharly commented Feb 16, 2024 •
edited
Loading

Description

Refactor and generalize success/error messages being used in the Snap-Keyring "adapter". This refactoring will ease additions of new metrics for Snap account management (future PR).

Related issues

Fixes: N/A

Manual testing steps

  1. Build and runs with yarn start:flask
  2. Use this test dapp: https://metamask.github.io/snap-simple-keyring/latest/
  3. Do each operations like create/remove an account.

Screenshots/Recordings

No changes to the UI/UX.

Screenshot 2024-02-22 at 16 42 46
Screenshot 2024-02-22 at 16 44 09

Screenshot 2024-02-22 at 16 43 04
Screenshot 2024-02-22 at 16 44 26

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • I've included manual testing steps
  • I've included screenshots/recordings if applicable
  • 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.
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • In case it's "ready for review", I've changed it from "draft" to "non-draft".

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-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Feb 16, 2024 •
edited
Loading

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.

@ccharly ccharly force-pushed the refactor/snap-keyring-ui-messages branch 2 times, most recently from e262a6e to 72a75f2 Compare February 19, 2024 08:54
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 19, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 28.00000% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 68.65%. Comparing base (61a79e0) to head (2137ce4).

Files Patch % Lines
app/scripts/lib/snap-keyring/snap-keyring.ts 16.67% 10 Missing ⚠️
app/scripts/lib/snap-keyring/utils/showResult.ts 37.50% 5 Missing ⚠️
app/scripts/lib/snap-keyring/utils/getSnapName.ts 40.00% 3 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #23007      +/-   ##
===========================================
- Coverage    68.65%   68.65%   -0.00%     
===========================================
  Files         1098     1100       +2     
  Lines        43185    43197      +12     
  Branches     11532    11532              
===========================================
+ Hits         29647    29655       +8     
- Misses       13538    13542       +4

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

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Feb 19, 2024

Builds ready [72a75f2]
Page Load Metrics (1011 ± 39 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1352732043919
domContentLoaded1599402813
load890119510118039
domInteractive1599402813
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -645 Bytes (-0.02%)
  • ui: 4.71 KiB (0.07%)
  • common: 0 Bytes (0.00%)

@ccharly
Copy link
Copy Markdown
Contributor Author

ccharly commented Feb 19, 2024

I have read the CLA Document and I hereby sign the CLA

@ccharly ccharly force-pushed the refactor/snap-keyring-ui-messages branch from 72a75f2 to 7504d02 Compare February 19, 2024 09:18
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Feb 19, 2024

Builds ready [7504d02]
Page Load Metrics (954 ± 64 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1134042006632
domContentLoaded985292412
load820149495413364
domInteractive985292412
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -645 Bytes (-0.02%)
  • ui: 4.71 KiB (0.07%)
  • common: 0 Bytes (0.00%)

@ccharly ccharly added the good first issue Good for newcomers label Feb 22, 2024
@ccharly ccharly mentioned this pull request Feb 22, 2024
Merged
33 tasks
@ccharly ccharly removed the good first issue Good for newcomers label Feb 22, 2024
@ccharly ccharly marked this pull request as ready for review February 22, 2024 15:12
@ccharly ccharly requested a review from a team as a code owner February 22, 2024 15:12
@ccharly ccharly force-pushed the refactor/snap-keyring-ui-messages branch 2 times, most recently from ed50d2f to ac743a4 Compare February 22, 2024 15:49
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Feb 22, 2024

Builds ready [ac743a4]
Page Load Metrics (1787 ± 56 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1184171967938
domContentLoaded17144383115
load15822102178711756
domInteractive17144383115
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -645 Bytes (-0.02%)
  • ui: 4.63 KiB (0.07%)
  • common: 0 Bytes (0.00%)

owencraston
owencraston reviewed Feb 22, 2024
View reviewed changes
owencraston
owencraston reviewed Feb 22, 2024
View reviewed changes
owencraston
owencraston reviewed Feb 22, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@owencraston owencraston left a comment

Choose a reason for hiding this comment

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

Overall this change is looking really good and I am in favour of the idea. I think this gives us a good opportunity to extract more logic out of the main snap-keyring.ts file as that file is getting chaotic. Let me know if you have any questions or if you need another pass.

@ccharly ccharly force-pushed the refactor/snap-keyring-ui-messages branch from 3a9266b to 5124640 Compare February 23, 2024 10:14
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Feb 23, 2024

Builds ready [5124640]
Page Load Metrics (1937 ± 88 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint11770022314369
domContentLoaded117831199
load16802364193718388
domInteractive117831199
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 505 Bytes (0.01%)
  • ui: 4.66 KiB (0.07%)
  • common: 0 Bytes (0.00%)

@ccharly ccharly force-pushed the refactor/snap-keyring-ui-messages branch from 5124640 to f9f11e8 Compare February 23, 2024 12:42
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Feb 23, 2024

Builds ready [f9f11e8]
Page Load Metrics (1845 ± 68 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1224101786330
domContentLoaded138535178
load15292074184514168
domInteractive138535178
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 505 Bytes (0.01%)
  • ui: 4.66 KiB (0.07%)
  • common: 0 Bytes (0.00%)

owencraston
owencraston previously approved these changes Feb 26, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@owencraston owencraston left a comment

Choose a reason for hiding this comment

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

Looks amazing. Thank you for breaking up the snap-keyring file 🙏

gantunesr
gantunesr reviewed Feb 27, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@gantunesr gantunesr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

From a discussion with @ccharly,

  • Add unit tests to the components SnapAccountSuccessMessage and SnapAccountErrorMessage
  • Add unit test to the method getSnapName

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Feb 27, 2024

Builds ready [d29b107]
Page Load Metrics (1300 ± 381 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint732471233517
domContentLoaded147131168
load5919421300793381
domInteractive147130168
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 505 Bytes (0.01%)
  • ui: 4.66 KiB (0.07%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Feb 27, 2024

Builds ready [d4fa44b]
Page Load Metrics (1054 ± 422 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint631771183115
domContentLoaded108131199
load5921321054879422
domInteractive108131199
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 505 Bytes (0.01%)
  • ui: 4.89 KiB (0.07%)
  • common: 0 Bytes (0.00%)

owencraston
owencraston previously approved these changes Feb 27, 2024
View reviewed changes
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Feb 28, 2024

Builds ready [67ee5de]
Page Load Metrics (1036 ± 378 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint652021263818
domContentLoaded1074382411
load5319101036787378
domInteractive1074382411
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 505 Bytes (0.01%)
  • ui: 4.89 KiB (0.07%)
  • common: 0 Bytes (0.00%)

owencraston
owencraston previously approved these changes Feb 28, 2024
View reviewed changes
@danroc danroc changed the title refactor(snap-keyring): better UI messages refactor(snap-account): refactor confirmation dialogs Feb 29, 2024
danroc
danroc previously approved these changes Feb 29, 2024
View reviewed changes
@danroc danroc changed the title refactor(snap-account): refactor confirmation dialogs refactor(snap-keyring): refactor confirmation dialogs Feb 29, 2024
gantunesr
gantunesr reviewed Feb 29, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

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

Looks good! Just left a final comment

@ccharly ccharly dismissed stale reviews from danroc and owencraston via f6a843f February 29, 2024 16:31
@ccharly ccharly force-pushed the refactor/snap-keyring-ui-messages branch from 67ee5de to f6a843f Compare February 29, 2024 16:31
danroc
danroc approved these changes Feb 29, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@danroc danroc left a comment

Choose a reason for hiding this comment

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

LGTM, but I'll leave this minor observation from our contributor doc:

Do not use "should" at the beginning of the test name. The official Jest documentation omits this word from their examples, and it creates noise when reviewing the list of tests printed after a run.

@gantunesr
Copy link
Copy Markdown
Member

gantunesr commented Feb 29, 2024

Interesting, I was not aware of that convention. Now that I think about it, having a more imperative tone in the test name does have a better ring to it. I'd push to take @danroc suggestion but I don't see it as blocker for this PR

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Mar 1, 2024

Builds ready [2137ce4]
Page Load Metrics (1220 ± 355 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint672391364421
domContentLoaded987352512
load5418501220739355
domInteractive987352512
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 505 Bytes (0.01%)
  • ui: 4.89 KiB (0.07%)
  • common: 0 Bytes (0.00%)

@ccharly ccharly merged commit d120d63 into develop Mar 1, 2024
@ccharly ccharly deleted the refactor/snap-keyring-ui-messages branch March 1, 2024 14:26
@github-actions github-actions bot locked and limited conversation to collaborators Mar 1, 2024
@metamaskbot metamaskbot added the release-11.13.0 Issue or pull request that will be included in release 11.13.0 label Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@gantunesr gantunesr gantunesr approved these changes

@danroc danroc danroc approved these changes

@owencraston owencraston owencraston left review comments

Assignees

No one assigned

Labels

area-snaps release-11.13.0 Issue or pull request that will be included in release 11.13.0 team-accounts-framework Accounts team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ccharly @metamaskbot @gantunesr @owencraston @danroc