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.

Sign up for GitHub

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

Jump to bottom

feat: use new SnapAuthorshipPill component to show snap origin in confirmation flow #26881

Merged
merged 16 commits into from
Sep 6, 2024
Merged

feat: use new SnapAuthorshipPill component to show snap origin in confirmation flow #26881

merged 16 commits into from
Sep 6, 2024

Conversation

hmalik88
Copy link
Contributor

@hmalik88 hmalik88 commented Sep 3, 2024 •
edited
Loading

Description

Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions:

  1. What is the reason for the change? Previously, the confirmation page would just show the snap id for "request from" as part of the confirmation screen.
  2. What is the improvement/solution? A pill is now used to show the snap icon and name, the pill opens up to a metadata modal.

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

Manual testing steps

  1. Build the extension
  2. Install a snap that uses personal_sign
  3. Call personal_sign from the snap

Screenshots/Recordings

Screen.Recording.2024-09-04.at.10.56.55.AM.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.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 3, 2024

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.

@github-actions github-actions bot added the team-snaps-platform Snaps Platform team label Sep 3, 2024
@hmalik88 hmalik88 changed the title feat: use new SnapAuthorshipPill component to show snap origin in personal sign feat: use new SnapAuthorshipPill component to show snap origin in personal_sign confirmation flow Sep 3, 2024
@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Sep 3, 2024
@hmalik88 hmalik88 removed the INVALID-PR-TEMPLATE PR's body doesn't match template label Sep 3, 2024
@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Sep 3, 2024
@hmalik88 hmalik88 marked this pull request as ready for review September 4, 2024 01:25
@hmalik88 hmalik88 requested review from a team as code owners September 4, 2024 01:25
@metamaskbot
Copy link
Collaborator

Builds ready [f762c4f]
Page Load Metrics (1746 ± 76 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint15642169175515374
domContentLoaded15342124172714570
load15622171174615776
domInteractive127132136
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 3.5 KiB (0.05%)
  • common: 0 Bytes (0.00%)

@codecov Codecov
Copy link

codecov bot commented Sep 4, 2024 •
edited
Loading

Codecov Report

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

Project coverage is 70.18%. Comparing base (b5a7b5c) to head (cf7c078).
Report is 2 commits behind head on develop.

Files with missing lines Patch % Lines
ui/components/app/confirm/info/row/url.tsx 80.00% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff            @@
##           develop   #26881   +/-   ##
========================================
  Coverage    70.18%   70.18%           
========================================
  Files         1426     1426           
  Lines        49661    49666    +5     
  Branches     13894    13895    +1     
========================================
+ Hits         34850    34854    +4     
- Misses       14811    14812    +1

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

@hmalik88 hmalik88 changed the title feat: use new SnapAuthorshipPill component to show snap origin in personal_sign confirmation flow feat: use new SnapAuthorshipPill component to show snap origin in confirmation flow Sep 4, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [01f794e]
Page Load Metrics (1681 ± 75 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint24720381542457220
domContentLoaded14471959165413464
load14552047168115675
domInteractive125532147
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 3.35 KiB (0.05%)
  • common: 0 Bytes (0.00%)

FrederikBolding
FrederikBolding previously approved these changes Sep 5, 2024
View reviewed changes
@FrederikBolding
Copy link
Member

Awesome work 🎉

@metamaskbot
Copy link
Collaborator

Builds ready [cf7c078]
Page Load Metrics (1808 ± 71 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint49722401747323155
domContentLoaded15832085178012862
load16152263180814971
domInteractive147332157
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 3.45 KiB (0.05%)
  • common: 0 Bytes (0.00%)

matthewwalsh0
matthewwalsh0 previously approved these changes Sep 6, 2024
View reviewed changes
ui/components/app/confirm/info/row/url.tsx Outdated Show resolved Hide resolved
ui/components/app/confirm/info/row/url.tsx Outdated Show resolved Hide resolved
ui/components/app/snaps/snap-authorship-pill/snap-authorship-pill.tsx Show resolved Hide resolved
matthewwalsh0
matthewwalsh0 previously approved these changes Sep 6, 2024
View reviewed changes
ui/components/app/confirm/info/row/url.tsx Outdated Show resolved Hide resolved
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Sep 6, 2024

Quality Gate Passed 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

@hmalik88 hmalik88 merged commit 8c06c97 into develop Sep 6, 2024
77 of 78 checks passed
@hmalik88 hmalik88 deleted the hm/update-confirmations-snap-origin-ui branch September 6, 2024 14:23
@github-actions github-actions bot locked and limited conversation to collaborators Sep 6, 2024
@metamaskbot metamaskbot added the release-12.6.0 Issue or pull request that will be included in release 12.6.0 label Sep 6, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [2b94344]
Page Load Metrics (1632 ± 65 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14261980163012861
domContentLoaded14191920161112359
load14271983163213565
domInteractive136632157
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 3.5 KiB (0.05%)
  • common: 0 Bytes (0.00%)

@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.
Reviewers

@matthewwalsh0 matthewwalsh0 matthewwalsh0 approved these changes

@FrederikBolding FrederikBolding FrederikBolding approved these changes

Assignees
No one assigned
Labels
INVALID-PR-TEMPLATE PR's body doesn't match template release-12.5.0 Issue or pull request that will be included in release 12.5.0 team-snaps-platform Snaps Platform team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hmalik88 @metamaskbot @FrederikBolding @matthewwalsh0 @gauthierpetetin