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

feat: Added UI for switching via dapp for custom chain id #26905

Merged
merged 6 commits into from
Sep 9, 2024

Conversation

NidhiKJha
Copy link
Member

@NidhiKJha NidhiKJha commented Sep 4, 2024

This PR is to update the UI when the dapp tries to add custom Chain Id network in MetaMask

Related issues

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

Manual testing steps

  1. Connect MetaMask to Uniswap
  2. Go to Uniswap console.
  3. Make an add ethereum chain call, the new popup should appear on confirmations screen

Screenshots/Recordings

Before

NA

After

Screen.Recording.2024-09-04.at.6.10.59.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 September 4, 2024 15:51
@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Sep 4, 2024
@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 Sep 4, 2024
bergeron
bergeron previously approved these changes Sep 4, 2024
@bergeron bergeron self-requested a review September 4, 2024 17:18
@bergeron
Copy link
Contributor

bergeron commented Sep 4, 2024

Hmmmm I got this error.

I didn't built with any env vars, just yarn; yarn webpack

Screen.Recording.2024-09-04.at.10.22.10.AM.mov

@gambinish
Copy link
Contributor

gambinish commented Sep 4, 2024

Hmmmm I got this error.

I didn't built with any env vars, just yarn; yarn webpack

Screen.Recording.2024-09-04.at.10.22.10.AM.mov

I got this same error, but after setting CHAIN_PERMISSIONS env var to true in .metamaskrc I saw the expected popup behavior as expected.

Should we handle the case where this flag is not set, rather than dumping a generic error?

@@ -249,6 +250,83 @@ function getValues(pendingApproval, t, actions, history, data) {
},
],
},
{
element: process.env.CHAIN_PERMISSIONS && 'BannerAlert',
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the expected behavior if this feature flag is not set?

@jonybur
Copy link
Contributor

jonybur commented Sep 5, 2024

Similarly to @gambinish, I get an error when not using the flag, but when the flag is on, everything works well.
Screenshot 2024-09-05 at 14 00 56
Screenshot 2024-09-05 at 14 02 42
If this is desired then we can move forward

@NidhiKJha
Copy link
Member Author

Nice catch @bergeron . @jonybur @gambinish without feature flag it should not crash. Here is the expected behaviour in both the cases

With feature flag

Screen.Recording.2024-09-09.at.3.45.24.AM.mov

Without feature flag

Screen.Recording.2024-09-09.at.3.47.48.AM.mov

@NidhiKJha NidhiKJha requested a review from gambinish September 9, 2024 02:48
@metamaskbot
Copy link
Collaborator

Builds ready [1786235]
Page Load Metrics (1641 ± 60 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint21521551366586281
domContentLoaded15222098162211957
load15402144164112660
domInteractive167035168
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 23 Bytes (0.00%)
  • common: 323 Bytes (0.00%)

Copy link

sonarcloud bot commented Sep 9, 2024

@metamaskbot
Copy link
Collaborator

Builds ready [c0a13ad]
Page Load Metrics (1665 ± 123 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint21026681536498239
domContentLoaded140826171650245118
load141626731665255123
domInteractive1395362110
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 23 Bytes (0.00%)
  • common: 323 Bytes (0.00%)

Copy link

codecov bot commented Sep 9, 2024

Codecov Report

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

Project coverage is 70.17%. Comparing base (838ad26) to head (c0a13ad).

Files with missing lines Patch % Lines
...tions/confirmation/templates/add-ethereum-chain.js 66.67% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #26905      +/-   ##
===========================================
- Coverage    70.17%   70.17%   -0.00%     
===========================================
  Files         1425     1425              
  Lines        49659    49662       +3     
  Branches     13891    13893       +2     
===========================================
+ Hits         34846    34848       +2     
- Misses       14813    14814       +1     

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

@NidhiKJha NidhiKJha merged commit 82f645e into develop Sep 9, 2024
78 checks passed
@NidhiKJha NidhiKJha deleted the added-26087 branch September 9, 2024 12:17
@github-actions github-actions bot locked and limited conversation to collaborators Sep 9, 2024
@metamaskbot metamaskbot added the release-12.6.0 Issue or pull request that will be included in release 12.6.0 label Sep 9, 2024
@gauthierpetetin 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
INVALID-PR-TEMPLATE PR's body doesn't match template needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. release-12.5.0 Issue or pull request that will be included in release 12.5.0 team-wallet-ux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants