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

feat: update dashboard nav item styles #9795

Merged
merged 7 commits into from
Jun 14, 2024
Merged

Conversation

nickoferrall
Copy link
Contributor

@nickoferrall nickoferrall commented May 29, 2024

Fix: #9790

Loom demo: https://www.loom.com/share/f2221c8aa804425791a442528c68bddb

Screenshot 2024-05-29 at 15 00 03

Do not merge this PR. I'll add further PRs to update the styles elsewhere, including mobile.

To test

  • Go to the dashboard on desktop and see that the UI looks like the designs

@nickoferrall
Copy link
Contributor Author

Do not merge this PR. I'll add further PRs to update the styles elsewhere, including mobile.

@ackernaut
Copy link
Member

@nickoferrall do you need anything else here or are you going to move on to another issue in the milestone?

@nickoferrall
Copy link
Contributor Author

@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

Copy link
Member

@ackernaut ackernaut left a 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!

@nickoferrall
Copy link
Contributor Author

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>
@github-actions github-actions bot added size/l and removed size/s labels Jun 14, 2024
@nickoferrall
Copy link
Contributor Author

nickoferrall commented Jun 14, 2024

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

Copy link
Member

@ackernaut ackernaut left a 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?

Screenshot 2024-06-14 at 12 09 38

@nickoferrall
Copy link
Contributor Author

@ackernaut did you do a git pull to get the latest from this branch? The same thing happened to me!

@ackernaut
Copy link
Member

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

@ackernaut
Copy link
Member

@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!

@nickoferrall
Copy link
Contributor Author

Thanks for the update and all of the reviews! Excited to see this in production!

@nickoferrall nickoferrall merged commit 71b17c2 into master Jun 14, 2024
6 checks passed
@nickoferrall nickoferrall deleted the feat/9790/nav-item-styles branch June 14, 2024 17:35
@github-actions github-actions bot mentioned this pull request Jun 24, 2024
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Admin Nav: Nav item styles
2 participants