-
Notifications
You must be signed in to change notification settings - Fork 331
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
feat: update dashboard nav item styles #9795
Conversation
Do not merge this PR. I'll add further PRs to update the styles elsewhere, including mobile. |
@nickoferrall do you need anything else here or are you going to move on to another issue in the milestone? |
@ackernaut nope, nothing else needed here. I'm moving on to the next issue in the milestone. I'll merge this once the other PRs are all ready |
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 this is in the right direction. I don’t know if you want to handle mobile styles just yet, but there’s a later of responsive styles that still need to be addressed at least to make the main nav section follow design. Happy to look at that, too!
For sure, I was planning on tackling that in a later PR. I wanted to keep this as small as possible |
Co-authored-by: Terry Acker <ackernaut@gmail.com>
Hey @ackernaut, this is the PR with the changes. If you're happy with it, we can merge to master and ship it to users! Loom demo here: https://www.loom.com/share/49477c45339a4b44ba29112872138d7d |
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 @nickoferrall ! For some reason I don’t think the series of PR merges worked correctly. When I run this feat/9790/nav-item-styles
locally it looks like very few of the changes made it to this branch. Am I using the wrong branch?
@ackernaut did you do a |
Oh 😓 I assumed this was a new branch at first, but then after seeing it was the original branch I didn’t think of that! Pulling now |
@nickoferrall this looks great! Nice work! I did test it locally with a branch including the global banner and everything looked right with the banner on or off for both desktop and mobile. I spotted a missing divider in the mobile nav, so made a small commit. Ready to merge! |
Thanks for the update and all of the reviews! Excited to see this in production! |
Fix: #9790
Loom demo: https://www.loom.com/share/f2221c8aa804425791a442528c68bddb
Do not merge this PR. I'll add further PRs to update the styles elsewhere, including mobile.
To test