-
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
chore: change default network avatar style #28976
Conversation
Builds ready [1eb299c]
Page Load Metrics (2058 ± 49 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [7b5b184]
Page Load Metrics (2004 ± 61 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [cf1d1e8]
Page Load Metrics (2135 ± 98 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [0d0fbbe]
Page Load Metrics (2233 ± 55 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
0d0fbbe
to
f62f604
Compare
Builds ready [f62f604]
Page Load Metrics (2288 ± 94 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
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 looking great! I’ve tried to check most instances of BadgeWrapper
that don’t use AvatarNetwork
to ensure that the changes to the defaults haven’t been overridden for those use cases, and so far, so good. I left one small question about the border color in the CSS; otherwise, I think it’s good to go.
I’d also like to understand how this change will be rolled out or planned for mobile and the portfolio. Ideally, they should be released at the same time or very close together to avoid inconsistencies across platforms.
ui/components/component-library/avatar-network/avatar-network.scss
Outdated
Show resolved
Hide resolved
ui/components/component-library/avatar-network/avatar-network.scss
Outdated
Show resolved
Hide resolved
ui/components/component-library/avatar-network/avatar-network.scss
Outdated
Show resolved
Hide resolved
@@ -52,6 +53,8 @@ export const AvatarNetwork: AvatarNetworkComponent = React.forwardRef( | |||
showHalo ? 'mm-avatar-network--with-halo' : '', | |||
className, | |||
)} | |||
borderRadius={BorderRadius.MD} | |||
borderWidth={1} |
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.
non-blocking: Because of how borders work in the DOM, adding this will effectively reduce the size of the AvatarNetwork
by 2px since the border will be included within the avatar size. I believe this has been adjusted in a few places, so it might not be an issue. However, we might need to account for this by applying box-sizing: content-box
.
box-sizing.720.mov
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 callout. The designs actually call for including the border within the avatar's size so I have removed the content-box
sizing from the AvatarNetwork
f62f604
to
06226c7
Compare
Builds ready [06226c7]
Page Load Metrics (1938 ± 74 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
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.
Description
This PR changes the default position of the network avatar to the bottomRight of the token. It also changes the default shape from a circle to a rounded square
Figma: https://www.figma.com/design/IuOIRmU3wI0IdJIfol0ESu/Cross-Chain-Swaps?node-id=7-24563&node-type=canvas&m=dev
Related issues
Fixes: N/A
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist