-
Notifications
You must be signed in to change notification settings - Fork 45
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
Add active state to mobile navigation buttons #1903
Conversation
lukaw3d
commented
Apr 24, 2024
•
edited
Loading
edited
Before | After |
---|---|
Deployed to Cloudflare Pages
|
Hey @donouwens, is that blue background (ROSE Wallet button) in dark theme correct? For me it does not match any other color used in mobile/ext view. |
// Make items mostly-equal width | ||
flex-grow: 1; | ||
flex-basis: 0; | ||
&:first-of-type { |
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.
what was a reason to change only first item? mimic white-space: nowrap;
?
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.
To make it 110% width compared to other items, due to text probably cutting out. But this only works on the extension width size. It would fail on tablet size I guess - not sure about when the mobile tabs are displayed exactly.
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.
what was a reason to change only first item? mimic white-space: nowrap;?
Yeah but it already seems to fit into one line now, so can remove
It should be the same blue indeed, please. |
Needed to also update font color to improve contrast.
eslint/prettier formatting |
box-shadow: ${({ theme }) => | ||
`0px 0px ${theme.global?.borderSize?.xsmall} ${normalizeColor('background-front-border', theme)}`}; | ||
border-top: ${({ theme }) => | ||
`solid ${theme.global?.borderSize?.xsmall} ${normalizeColor('background-contrast', theme)}`}; | ||
flex-direction: row; | ||
` | ||
|
||
const StyledNavLink = styled(NavLink)` | ||
// Make items mostly-equal width |
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 comment has no context now
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.
should be "Make items equal width" now
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 think comment is not needed at all, as it is straight forward from the code.
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.
flex-basis: 0;
seemed unintuitive to me :/ I wouldn't need a comment if it was grid-template-columns: 1fr 1fr 1fr 1fr;