-
Notifications
You must be signed in to change notification settings - Fork 27
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
501/navbar hamburger menu and notification bell improvements #548
501/navbar hamburger menu and notification bell improvements #548
Conversation
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 @richestrinh thanks for contributing to changes for PASS! The changes for this PR looks great, just a few details I want this PR to go over. (see feedback)
One other thing that could be addressed here or another PR is the extra padding from the MUI menu and MUI menu list (see clip). If we could get the padding away, think it'll be good.
Screen.Recording.2023-11-28.at.17.23.45.mov
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.
After Ka Hung's questions/concerns are addressed, I would approve as well. Don't see anything else not already mentioned.
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 other than what I've just noticed, think every else looks alright. Think it'll be good to go after the issue with the padding gets resolved (see new comment).
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.
Oh, this is nice! We could of course go back in later and fix the padding of the notification menu once we got a working design (and not a placeholder), but I think the NavMenu is looking pretty great now.
Can certainly approve this for merging. Good 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.
I approved, and then realized there is a difference on full mobile menu (when contacts and civic profile are also in the menu.)
The padding isn't consistent on that one compared to the others that were changed.
I think it's still the double padding version.
I think it had to do with the min-height for the mobile view. |
…ation-bell-improvements
This PR:
Resolves #501
1. Updated positioning for hamburger menu and bell drop-down to appear below nav bar.
2. Updated backdrop for hamburger menu and bell.
3. Fixed hamburger menu icons to have consistent sizing.
4. Updated spacing around divider seperating routes and settings.
Screenshots (if applicable):