-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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: replace deprecated chip component with tag #24594
base: develop
Are you sure you want to change the base?
feat: replace deprecated chip component with tag #24594
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. |
Hey @nickpismenkov, could you please make sure the PR description is complete and includes the manual testing steps for the reviewers? Thanks! |
@georgewrmarshall Hi. I've completed the description. Could you please help with e2e failed tests? |
…ory for recovery phrase chips
import Chip from '../../../components/ui/chip'; | ||
import Box from '../../../components/ui/box'; | ||
import Typography from '../../../components/ui/typography'; |
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.
Replacing more deprecated components
TypographyVariant, | ||
TextVariant, | ||
BorderStyle, | ||
Size, | ||
DISPLAY, | ||
BorderRadius, | ||
Display, |
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.
Replacing deprecated helpers
phraseRevealed: false, | ||
}; | ||
|
||
export const PhraseRevealed = Template.bind({}); |
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.
Adding a Storybook story for easier isolated development
sb720.mov
…ining visual integrity
&__chips { | ||
display: grid; | ||
justify-items: center; | ||
align-items: center; | ||
row-gap: 16px; | ||
grid-template-columns: repeat(auto-fill, minmax(160px, 1fr)); | ||
|
||
@include design-system.screen-sm-min { | ||
grid-template-columns: 1fr 1fr 1fr; | ||
} | ||
|
||
&--hidden { | ||
filter: blur(5px); | ||
} | ||
} | ||
|
||
&__secret { | ||
position: relative; | ||
width: 100%; | ||
} | ||
|
||
&__secret-blocker { | ||
position: absolute; | ||
top: 0; | ||
bottom: 0; | ||
height: 100%; | ||
width: 100%; | ||
background-color: var(--color-overlay-alternative); | ||
display: flex; | ||
flex-flow: column nowrap; | ||
align-items: center; | ||
justify-content: center; | ||
padding: 8px 0 18px; | ||
border-radius: 4px; | ||
color: var(--color-overlay-inverse); | ||
|
||
&--text { | ||
margin-top: 32px; | ||
} | ||
} | ||
|
||
&__chip-item { | ||
display: flex; | ||
flex-direction: row; | ||
align-items: center; | ||
text-align: center; | ||
|
||
&__number { | ||
font-size: design-system.$font-size-h5; | ||
} | ||
} | ||
|
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.
Removing unused styles and moving some to https://github.com/MetaMask/metamask-extension/pull/24594/files#diff-58ef2d79c613500bbe5a15545874fbeb5a4d07a22d3796a9e9a1d9463a592347
@@ -19,7 +19,7 @@ describe('Recovery Phrase Chips Component', () => { | |||
|
|||
const recoveryPhraseChips = queryByTestId('recovery-phrase-chips'); | |||
expect(recoveryPhraseChips).not.toHaveClass( | |||
'recovery-phrase__chips--hidden', | |||
'recovery-phrase-chips__chips--hidden', |
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.
Updating classnames to reflect whats in javascript
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.
Hey @nickpismenkov,
I did some more digging, and I think this task is a bit more complex than I initially thought. I've refactored it to align with our design system standards.
I've created a couple of other PRs that will need to be merged before this one:
- docs: adding recover-phrase-stories #24781
- fix: adding missing overlay alternative color to helpers #24780
Once the above PRs are merged, we can rebase and re-test. Thanks for your contribution and patience.
Blocking until above PRs have been merged
Description
Replaced deprecated chip component with tag in recovery-phrase-chips component
Related issues
Fixes: #20487
Manual testing steps
Screenshots/Recordings
Before
After
after720.mov
Pre-merge author checklist
Pre-merge reviewer checklist