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(masthead): added horizontal nav demo #4617

Merged
merged 2 commits into from
Jan 19, 2022

Conversation

mattnolting
Copy link
Contributor

@mattnolting mattnolting commented Jan 18, 2022

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.

  • Do we want to show this demo with context selector included? It has to live in the same container as __brand to be accommodated.
  • Should the nav be full height?
  • Any design details that I've missed...

@patternfly-build
Copy link

patternfly-build commented Jan 18, 2022

Copy link
Member

@mcarrano mcarrano left a 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.

Copy link
Member

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

Copy link
Member

@srambach srambach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

L🔥TM!

@mcoker mcoker merged commit 9fec1a0 into patternfly:main Jan 19, 2022
@patternfly-build
Copy link

🎉 This PR is included in version 4.169.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add nav to masthead example/demo
5 participants