-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Refactor AccountConnection with new layout and type components
#7345
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
Conversation
size-limit report 📦
|
|
|
||
| const termsOfServiceMarkup = termsOfService ? ( | ||
| <div className={styles.TermsOfService}>{termsOfService}</div> | ||
| <Box paddingTop="5">{termsOfService}</Box> |
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.
Shouldn't we be using tokens, rather than hardcoded numbers?
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.
Oh, wait. I see. This is a number from the set made available in the Box component props. 🤦
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.
Yes it maps to our spacing tokens. But perhaps it's not very clear? I wonder if there's anything we can do to make that easier?
### WHY are these changes introduced? #7345 Refactors the `AccountConnection` component to use the new layout primitives including `AlphaCard`. `AlphaCard` uses `useBreakpoints()` which required setting up a mock to set media width for the tests. It appears setting the media width in the tests becomes necessary for any parent components using a component that uses `useBreakpoints()`. ### WHAT is this pull request doing? This adds a `setMediaWidth` test utility which can be used in tests for any components that require mocked media widths in their tests. Co-authored-by: Aaron Casanova <32409546+aaronccasanova@users.noreply.github.com>
3468945 to
db25106
Compare
db25106 to
69e2dfc
Compare
laurkim
left a comment
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.
Nice!! Changes on Storybook lgtm 🚀
### WHY are these changes introduced? Fixes #7341 <!-- link to issue if one exists -->
### WHY are these changes introduced? Fixes #7341 <!-- link to issue if one exists -->
### WHY are these changes introduced? Fixes #7341 <!-- link to issue if one exists -->
### WHY are these changes introduced? Fixes #7341 <!-- link to issue if one exists -->
Fixes #7341 <!-- link to issue if one exists -->
Fixes #7341 <!-- link to issue if one exists -->
Fixes #7341 <!-- link to issue if one exists -->
Fixes #7341 <!-- link to issue if one exists -->
Fixes #7341 <!-- link to issue if one exists -->
Fixes #7341 <!-- link to issue if one exists -->
WHY are these changes introduced?
Fixes #7341