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

chore: MMI adds back the current Tx confirmation view to MMI #26539

Merged
merged 12 commits into from
Aug 27, 2024

Conversation

zone-live
Copy link
Contributor

Description

While we don't support the new confirmation UI for Txs (MMI flow got broken with the upgrade), we need to show the current one to MMI.

Related issues

Fixes:

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

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.

@zone-live zone-live added the team-mmi PRs from the MMI team label Aug 20, 2024
@zone-live zone-live requested a review from a team as a code owner August 20, 2024 14:26
Copy link
Contributor

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.

@metamaskbot
Copy link
Collaborator

Builds ready [567318f]
Page Load Metrics (78 ± 13 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint721871062512
domContentLoaded41174702814
load48179782813
domInteractive89828199
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 7 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@zone-live zone-live requested a review from a team as a code owner August 21, 2024 14:22
@metamaskbot
Copy link
Collaborator

Builds ready [cfc91f6]
Page Load Metrics (84 ± 14 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint692791214723
domContentLoaded47175793014
load54175842814
domInteractive167730168
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 7 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link

codecov bot commented Aug 21, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.04%. Comparing base (ab79f0d) to head (7428e1e).
Report is 15 commits behind head on develop.

Files Patch % Lines
...ages/confirmations/hooks/useCurrentConfirmation.ts 66.67% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #26539      +/-   ##
===========================================
+ Coverage    70.02%   70.04%   +0.02%     
===========================================
  Files         1408     1411       +3     
  Lines        49105    49209     +104     
  Branches     13735    13756      +21     
===========================================
+ Hits         34383    34466      +83     
- Misses       14722    14743      +21     

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

@@ -31,6 +31,10 @@ export function isBeta() {
return process.env.METAMASK_BUILD_TYPE === 'beta';
}

export function isMMI() {
Copy link
Contributor Author

@zone-live zone-live Aug 22, 2024

Choose a reason for hiding this comment

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

@matthewwalsh0 This seems to be the logical place for what you suggested, already does it for the beta build type. I'm not adding a code fence for this since it would mean that an other one would have to exist on the useCurrentConfirmation hook (otherwise things break), also it would add unnecessary complexity imo and this can be useful down the road for similar cases.

@metamaskbot
Copy link
Collaborator

Builds ready [6e148ae]
Page Load Metrics (77 ± 10 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint702141072914
domContentLoaded47151722211
load54151772010
domInteractive1610131178
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 158 Bytes (0.00%)
  • common: 29 Bytes (0.00%)

Copy link

@metamaskbot
Copy link
Collaborator

Builds ready [7428e1e]
Page Load Metrics (79 ± 8 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint761591112311
domContentLoaded4611575189
load5411579178
domInteractive115927115
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 158 Bytes (0.00%)
  • common: 29 Bytes (0.00%)

@zone-live zone-live merged commit 5b6e7ef into develop Aug 27, 2024
78 checks passed
@zone-live zone-live deleted the MMI-5392-revert-new-confirmations-ui-to-current branch August 27, 2024 15:57
@github-actions github-actions bot locked and limited conversation to collaborators Aug 27, 2024
@metamaskbot metamaskbot added the release-12.5.0 Issue or pull request that will be included in release 12.5.0 label Aug 27, 2024
@gauthierpetetin gauthierpetetin added release-12.4.0 Issue or pull request that will be included in release 12.4.0 and removed release-12.5.0 Issue or pull request that will be included in release 12.5.0 labels Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.4.0 Issue or pull request that will be included in release 12.4.0 team-mmi PRs from the MMI team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants