-
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: Support various redesign signatures in ConfirmPage Storybook #25054
feat: Support various redesign signatures in ConfirmPage Storybook #25054
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. |
Builds ready [9b804ae]
Page Load Metrics (368 ± 327 ms)
Bundle size diffs
|
Builds ready [9b804ae]
Page Load Metrics (368 ± 327 ms)
Bundle size diffs
|
Builds ready [9b804ae]
Page Load Metrics (368 ± 327 ms)
Bundle size diffs
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #25054 +/- ##
========================================
Coverage 65.61% 65.61%
========================================
Files 1365 1365
Lines 54224 54224
Branches 14180 14180
========================================
Hits 35576 35576
Misses 18648 18648 ☔ View full report in Codecov by Sentry. |
const ConfirmPageStory = { | ||
title: 'Pages/Confirm/ConfirmPage', | ||
decorators: [(story) => <Provider store={store}>{story()}</Provider>], | ||
decorators: [(story) => <div style={{ height: '600px' }}>{story()}</div>], |
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.
Should we also force the width so it matches the standard MetaMask popup?
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.
Good question. As you might have noticed, the height resembles our popup height. I added this to view the scrollToBottom feature on some of the signature pages without needing to hardcode or add a control for this.
I left out the width so that we could easily test various widths. These decisions were subjective. Open to other suggestions
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.
Could we still support re-sizing to test wrapping etc, but default to the standard width?
8510dfc
Builds ready [1076e3b]
Page Load Metrics (49 ± 4 ms)
Bundle size diffs
|
Description
This PR adds support for various redesign signatures for the ConfirmPage storybook.
Related issues
Fixes: #25052
Manual testing steps
yarn storybook
Screenshots/Recordings
Before
After
CleanShot.2024-06-05.at.18.35.43-converted.mp4
higher quality of above:
https://github.com/MetaMask/metamask-extension/assets/20778143/342a6182-c443-4bd9-907a-3441b4b9eb81
Pre-merge author checklist
Pre-merge reviewer checklist