-
Notifications
You must be signed in to change notification settings - Fork 171
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 reduced top navigation bar #5107
Conversation
344f348
to
3227e6c
Compare
@petesfrench @jmuzina @pastelcyborg While this is not fully ready for review (docs are missing), code should be more or less in required shape. Please have a look if you get a chance and leave any comments, suggestions, questions that may arise. Pete has the most context, but all comments are welcome. This is just a part of a meganav, so techcally is not meant to be fully working (for example empty dropdowns), but let's make it as good as possible for Vanilla standard. |
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 needs a package.json
minor version bump, as well as an entry in releases.yaml
for release notes
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.
Thanks for doing this, @petesfrench!
High-level comment/question: I notice the reduced nav on mobile is positioned statically - it pushes page content down when opened. Just wanted to confirm this is desired functionality? Sometimes this shift can be quite jarring.
Is there meant to be so much whitespace between the topnav and the secondary nav on mobile when the topnav is expanded? Screencast.from.07-01-2024.10.13.58.AM.webmEdit: No, this scrolling is not intentional, seems like a bug with sidenav: |
No, good catch. Looks like we have this issue with standard sliding nav as well. Body elements need to be set to overflow hidden (disabled scroll) when sliding nav is open. I need to figure out how to best do it (JS, CSS?). |
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.
Great work, that was quite an effort :)
Good to go pending design review
Sliding mobile navigation has not been part of this PR. Animating this (as discussed) will probably increase the scope, but I can try.
Icons in light/dark theme match default text colour. We don't have muted icons. This would be out of scope for this PR, we would need to solve it on theme level (and have 2x more icons defined - muted versions of everything).
Out of scope for this PR. This is about the navigation itself, not the dropdowns in meganav.
This is caused by default Vanilla navigation breakpoint not being large enough for this. If we change it, it will change for everyone by default, as it's a settings level.
This is not a menu - you have search open. Search icon changes to X icon for that one. |
As discussed in stand-up, we are merging this as-is for now, but the visual issues will be addressed in following updates. |
Done
is-sticky
class nameFixes https://warthogs.atlassian.net/browse/WD-11789
QA
Note: This is part of the work to upstream meganav functionality and may not be fully functional on its own.
Currently we focus mostly on the styling of the reduced navigation on desktop screens, making sure it doesn't break on mobile, that search is opened, that the secondary navigation below it works.
The contents of the dropdowns opened by reduced navigation are not part of this, the dropdowns funcionality on mobile is also not part of this (the "sliding" navigation).
Check if PR is ready for release
If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:
Feature 🎁
,Breaking Change 💣
,Bug 🐛
,Documentation 📝
,Maintenance 🔨
.package.json
should be updated relative to the most recent release, following semver convention:Screenshots