Skip to content
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

Merged
merged 5 commits into from
Apr 26, 2024
Merged

Add active state to mobile navigation buttons #1903

merged 5 commits into from
Apr 26, 2024

Conversation

lukaw3d
Copy link
Member

@lukaw3d lukaw3d commented Apr 24, 2024

Before After
localhost_3000_account_oasis1qrtyn2q78jv6plrmexrsrv4dh89wv4n49udtg2km(extension375x600) localhost_3000_account_oasis1qrtyn2q78jv6plrmexrsrv4dh89wv4n49udtg2km_fiat(extension375x600)

@lukaw3d lukaw3d requested review from buberdds and lubej April 24, 2024 20:48
Copy link

github-actions bot commented Apr 24, 2024

Deployed to Cloudflare Pages

Latest commit: 75cb18872d32a7a59e2600bec9dc33ab83e8ba28
Status:✅ Deploy successful!
Preview URL: https://1331b69c.oasis-wallet.pages.dev

@buberdds
Copy link
Contributor

buberdds commented Apr 25, 2024

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.

Screenshot from 2024-04-25 11-13-14

// Make items mostly-equal width
flex-grow: 1;
flex-basis: 0;
&:first-of-type {
Copy link
Contributor

@buberdds buberdds Apr 25, 2024

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;?

Copy link
Collaborator

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.

Copy link
Member Author

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

@donouwens
Copy link

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.

It should be the same blue indeed, please.

lukaw3d added a commit that referenced this pull request Apr 26, 2024
@buberdds
Copy link
Contributor

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
Copy link
Collaborator

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

Copy link
Member Author

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

Copy link
Collaborator

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.

Copy link
Member Author

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;

@lukaw3d lukaw3d merged commit 32c74fb into master Apr 26, 2024
13 checks passed
@lukaw3d lukaw3d deleted the lw/mobile-nav branch April 26, 2024 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants