-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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: Custom header for wallet initiated confirmations #27391
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. |
04439b5
to
b48df06
Compare
db7249e
to
e6f017a
Compare
Quality Gate failedFailed conditions See analysis details on SonarCloud Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
Builds ready [0697807]
Page Load Metrics (1778 ± 106 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
borderRadius={BorderRadius.MD} | ||
marginRight={1} | ||
> | ||
<ButtonIcon |
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.
Minor, is there some duplication here with HeaderInfo
, is it worth a AdvancedDetailsToggleButton
component somewhere?
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.
This is a good idea, I'll add it in the next PR
(currentConfirmation as TransactionMeta).origin === 'metamask'; | ||
|
||
if (isWalletInitiated) { | ||
return <WalletInitiatedHeader />; |
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.
Minor, as an alternate to returning an enitrely separate component here, is there much overlap between the two headers such that it's worth just dynamically including the back button etc?
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.
I think the components are sufficiently different to warrant separate components
Description
The back button brings the user to the stepper for ERC20 token transfer.
This PR includes creating a placeholder component for token transfer confirmations. It also includes unit tests.
Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3218
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist