-
Notifications
You must be signed in to change notification settings - Fork 5k
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: Add full screen Snap Home and Dialog #25670
Conversation
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. |
117bcd1
to
7bd3b9a
Compare
ba61c9c
to
6cbd3d7
Compare
Builds ready [369a1ff]
Page Load Metrics (392 ± 367 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #25670 +/- ##
===========================================
- Coverage 69.75% 69.74% -0.01%
===========================================
Files 1400 1400
Lines 49419 49428 +9
Branches 13668 13673 +5
===========================================
+ Hits 34470 34472 +2
- Misses 14949 14956 +7 ☔ View full report in Codecov by Sentry. |
369a1ff
to
0073e3f
Compare
Builds ready [0073e3f]
Page Load Metrics (242 ± 238 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [1ba09e8]
Page Load Metrics (65 ± 9 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
1ba09e8
to
d1d76a2
Compare
Builds ready [d1d76a2]
Page Load Metrics (416 ± 314 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Remove duplicate header and fix lint
This reverts commit 57ead65.
8b31e04
to
1084cc0
Compare
Quality Gate passedIssues Measures |
Builds ready [1084cc0]
Page Load Metrics (147 ± 169 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
onCancel={ | ||
templatedValues.onCancel || | ||
// /!\ Treat cancel as submit only if approval type is appropriate /!\ | ||
(pendingConfirmation?.type === ApprovalType.SnapDialogAlert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than adding further coupling to specific types in this generic component, could we instead just specify a suitable onReject
in the relevant templates such as ui/pages/confirmations/confirmation/templates/snaps/snap-alert/snap-alert.js
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with this is when onCancel
is added to the snap-alert.js
, the confirmation popup renders an extra button. Alert needs to have only one button by design. We just want to have the possibility to close popup from header everywhere (in all Snaps dialogs) and this case was somehow exception because there is no reason to have two buttons on alert screen.
Do you see quick fix for this maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be too much extra scope for now, but we could add a hideCancelButton
property to the template to match hideSubmitButton
so you could still define an onCancel
but explicitly hide the button so it's only used for this header exit behaviour.
onCancel={ | ||
templatedValues.onCancel || | ||
// /!\ Treat cancel as submit only if approval type is appropriate /!\ | ||
(pendingConfirmation?.type === ApprovalType.SnapDialogAlert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be too much extra scope for now, but we could add a hideCancelButton
property to the template to match hideSubmitButton
so you could still define an onCancel
but explicitly hide the button so it's only used for this header exit behaviour.
Description
This PR adds redesigned UI and new UX to the Snap home page and Snap dialog. Snap Authorship header is also updated and its new UX/UI reflected across the related pages.
Related issues
Fixes: #25068
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist