-
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: new receive flow #26148
feat: new receive flow #26148
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 [57abcd1]
Page Load Metrics (84 ± 8 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
} | ||
|
||
&__message { | ||
@include design-system.H7; |
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 realize we need to add some stylelinting to deprecated these but the typography mixins have been deprecated in favor of the text component can we use it here instead?
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.
Just removed &__message since it is unused
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.
Great use of component library components! Left some non-blocking comments.
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! But it shouldn't conflict with the existing import account test?
@@ -162,40 +161,6 @@ describe('Import flow @no-mmi', function () { | |||
); | |||
}); | |||
|
|||
it('Import wallet using Secret Recovery Phrase with pasting word by word', async function () { |
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.
Why did we remove this test
Co-authored-by: George Marshall <george.marshall@consensys.net>
Co-authored-by: George Marshall <george.marshall@consensys.net>
…ion into jb-receive-flow
Quality Gate passedIssues Measures |
Builds ready [36fc160]
Page Load Metrics (78 ± 7 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
@@ -38,7 +38,7 @@ describe('BTC Account - Overview', function (this: Suite) { | |||
assert.equal(await buySellButton.isEnabled(), false); | |||
|
|||
const portfolioButton = await driver.waitForSelector( | |||
'[data-testid="coin-overview-portfolio"]', | |||
'[data-testid="coin-overview-receive"]', |
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 spec is called has portfolio button enabled for BTC accounts
however, here we are checking if the receive button is enabled (not the portfolio).
The portfolio link doesn't appear anymore. Is this expected? If so, the title for the test should be changed, otherwise we should open a bug, indicating that the portfolio link doesn't appear anymore
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 catch, PR to fix this:
#28184
Description
Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2383
Manual testing steps
Screenshots/Recordings
demo.mp4
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist