-
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
fix(28081): design tweak for network badge #29324
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. |
backgroundColor={BackgroundColor.backgroundDefault} | ||
borderWidth={2} |
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.
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 @DDDDDanica, should the before and after images be swapped? For example, should the 2px border be after and the 1px border be before?
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 spot ! switched 🙏🏻
@@ -74,7 +74,7 @@ export const BadgeStatus: React.FC<BadgeStatusProps> = ({ | |||
backgroundColor={badgeBackgroundColor} | |||
borderRadius={BorderRadius.full} | |||
borderColor={badgeBorderColor} | |||
borderWidth={isConnectedAndNotActive ? 2 : 4} | |||
borderWidth={2} |
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.
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.
@@ -90,7 +90,7 @@ export const ConnectedSiteMenu = ({ | |||
? BorderColor.successDefault | |||
: BackgroundColor.backgroundDefault | |||
} | |||
borderWidth={isConnectedtoOtherAccountOrSnap ? 2 : 3} | |||
borderWidth={2} |
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.
@@ -62,7 +62,7 @@ import { | |||
getMultichainShouldShowFiat, | |||
} from '../../../selectors/multichain'; | |||
import { useMultichainAccountTotalFiatBalance } from '../../../hooks/useMultichainAccountTotalFiatBalance'; | |||
import { ConnectedStatus } from '../connected-status/connected-status'; | |||
import { ConnectedStatus } from '../connected-status'; |
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.
Path simplification
2311b79
to
1061690
Compare
Builds ready [1061690]
Page Load Metrics (1747 ± 68 ms)
Bundle size diffs
|
1061690
to
573e5a7
Compare
Builds ready [573e5a7]
Page Load Metrics (1862 ± 212 ms)
Bundle size diffs
|
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! 🔥 I made some small updates hope that is ok!
@@ -476,7 +476,7 @@ exports[`SnapUIAddress renders legacy Ethereum address 1`] = ` | |||
<rect | |||
fill="#F29602" | |||
height="32" | |||
transform="translate(-2.81750896228304 4.951916544006229) rotate(230.4 16 16)" | |||
transform="translate(-2.8175089622830405 4.951916544006229) rotate(230.4 16 16)" | |||
width="32" | |||
x="0" | |||
y="0" |
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 found that these snapshots update regardless of changes not sure if we want to include them or not 🤔
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 found similar thing in weird svg snapshot failure, FAIL ui/components/multichain/connect-accounts-modal/connect-accounts-modal.test.tsx (5.461 s), and this one only failed in github actions
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.
Trying to manually applying those snapshot changes locally and see how it goes 🙇🏻♀️
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.
Addressed: c27d151
@@ -42,7 +42,7 @@ export const DefaultStory = Template.bind({}); | |||
|
|||
export const NotConnectedStory = Template.bind({}); | |||
NotConnectedStory.args = { | |||
badgeBackgroundColor: Color.borderMuted, | |||
badgeBackgroundColor: BackgroundColor.iconAlternative, |
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.
@@ -74,7 +74,7 @@ export const BadgeStatus: React.FC<BadgeStatusProps> = ({ | |||
backgroundColor={badgeBackgroundColor} | |||
borderRadius={BorderRadius.full} | |||
borderColor={badgeBorderColor} | |||
borderWidth={isConnectedAndNotActive ? 2 : 4} | |||
borderWidth={2} |
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.
let badgeBackgroundColor = Color.borderMuted; // //TODO: Replace it once Background color has this value. | ||
let badgeBorderColor = BorderColor.backgroundDefault; // TODO: Replace it once border-color has this value. | ||
let badgeBackgroundColor = BackgroundColor.iconAlternative; |
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.
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.
Thanks George ! that makes sense !
@@ -43,6 +43,7 @@ const DetectedTokenDetails = ({ | |||
src={CHAIN_ID_TO_NETWORK_IMAGE_URL_MAP[chainId]} | |||
name={currentNetwork?.nickname || ''} | |||
backgroundColor={testNetworkBackgroundColor} | |||
borderWidth={2} |
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.
backgroundColor={BackgroundColor.backgroundDefault} | ||
borderWidth={2} |
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 @DDDDDanica, should the before and after images be swapped? For example, should the 2px border be after and the 1px border be before?
a8ccba3
to
d224ed3
Compare
d224ed3
to
a2d9da4
Compare
Builds ready [c27d151]
Page Load Metrics (1515 ± 59 ms)
Bundle size diffs
|
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 work!
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
Builds ready [b7f2147]
Page Load Metrics (1880 ± 66 ms)
Bundle size diffs
|
Description
This PR address feedback from design quality check to tweak board color and width for network badge.
Related issues
Fixes: #28081
Manual testing steps
Screenshots/Recordings
Before
See PR for related changes
After
See PR for related changes
Pre-merge author checklist
Pre-merge reviewer checklist