Skip to content

Conversation

@tlabaj
Copy link
Contributor

@tlabaj tlabaj commented Jun 1, 2023

What: Closes #9015

@patternfly-build
Copy link
Contributor

patternfly-build commented Jun 1, 2023

Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

For the Horizontal Nav demo, the breakpoints aren't kicking in at the right time resulting in content overflowing to the right when the viewport is shrunk down:

Horizontal demo overflow

The Horizontal Nav with Horizontal Subnav demo also has a similar overflow issue, except it's overflowing on page load.

We could adjust the visibility of the ToolbarGroup's and set a width for the Nav components in these two examples.

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 still has the issue of the user menu having a lighter background in dark theme.

Screenshot 2023-06-02 at 3 45 29 PM

@mcoker I thought that was fixed?

@mcoker
Copy link
Contributor

mcoker commented Jun 2, 2023

@mcarrano I have a PR up that fixes it, but it didn't go in until late last night and it was too late to merge - patternfly/patternfly#5643. If we want to try and get it in, I have an open question in the issue here - patternfly/patternfly#5643 (comment)

@mcarrano
Copy link
Member

mcarrano commented Jun 5, 2023

Thanks @mcoker I would try to get this in if we can. As to your question on patternfly/patternfly#5643, I'm not sure what the right answer is and want to get input from @mceledonia . Will reach out on Slack to resolve this ASAP.

@nicolethoen can we still try to get this into the release? The user menu does not look correct without it in dark mode.

@tlabaj
Copy link
Contributor Author

tlabaj commented Jun 6, 2023

@thatblindgeye @mcoker it looks like we are missing a "pf-m-overflow-container" on the toolbar item that holds the nav. React does not currently support that. I can add it as part of this PR. It should fix that issue @thatblindgeye mentions above

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.

👍👍

Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Just a quick update below

@mcarrano
Copy link
Member

mcarrano commented Jun 6, 2023

User menu looks good now. Just seeing a couple of additional issues.

@tlabaj
Copy link
Contributor Author

tlabaj commented Jun 6, 2023

User menu looks good now. Just seeing a couple of additional issues.

@mcarrano there is an issue open for drilldown nav here #9183

I updated the demo titles

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.

Thanks @tlabaj . I am good to merge this.

Copy link
Contributor

@kmcfaul kmcfaul left a comment

Choose a reason for hiding this comment

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

lgtm.

I did notice that the nav drilldown demo looks like it may be broken, but this PR didn't touch that so we may want a follow up issue.

@kmcfaul kmcfaul merged commit 1eeac6f into patternfly:main Jun 7, 2023
nicolethoen pushed a commit to Kells512/patternfly-react that referenced this pull request Sep 1, 2023
… of the page header (patternfly#9223)

* chore(docs): Updated the Navigation demos to use the masthead instead of the page header

* Updates from comments and added suport for toolbar item/group overflow container

* update comments from review

* remove legacy refs

* fix sentence case

---------

Co-authored-by: Titani <tlabaj@redaht.com>
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.

Navigation demos - update usage of PageHeader

6 participants