-
Notifications
You must be signed in to change notification settings - Fork 99
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(masthead): added horizontal nav demo #4617
Conversation
fab6cd6
to
e465e0b
Compare
Preview: https://patternfly-pr-4617.surge.sh A11y report: https://patternfly-pr-4617-a11y.surge.sh |
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.
@mattnolting Yes, there are a few things that should change here:
- We wouldn't recommend using this together with a vertical nav, so I think you should take that out.
- I'd be in favor of removing the context selector here. I don't think that we'd recommend having both that and a horizontal menu live together in the masthead.
- I believe that our design intent was to have the menu fill the full height of the masthead (I'm guessing that's what you mean by "full height).
@mceledonia do you agree with the above? Am I missing anything? The bottom line is that we rarely use a horizontal primary navigation and would only recommend it in a very simple use case which is why you don't see it often in product.
e465e0b
to
9df938e
Compare
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 looks great. Thanks @mattnolting !
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.
LGTM
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.
L🔥TM!
🎉 This PR is included in version 4.169.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
closes #4522
@mcarrano @mceledonia can you please review Masthead with horizontal nav? A couple of things in question, bearing in mind that space becomes much more limited in this iteration.
__brand
to be accommodated.