fix(nav): fix horizontal nav spacing, background color #1798
Conversation
|
Deploy preview for pf-next ready! Built with commit 03f9f27 |
| --pf-c-page__header-nav--lg--PaddingLeft: 0; | ||
| --pf-c-page__header-nav--lg--MarginLeft: var(--pf-global--spacer--xl); | ||
| --pf-c-page__header-nav--lg--MarginRight: var(--pf-global--spacer--xl); | ||
| --pf-c-page__header-nav--BackgroundColor: var(--pf-global--BackgroundColor--dark-transparent-100); | ||
| --pf-c-page__header-nav--BackgroundColor: #242424; // Custom color for this element only | ||
| --pf-c-page__header-nav--lg--BackgroundColor: transparent; |
There was a problem hiding this comment.
Do we need to set this variable, could you change the breakpoint to max-width and just set the background color there. I know its a breaking change but maybe we could create an issue to remove it?
There was a problem hiding this comment.
Doesn't matter to me. It's probably the same amount of code either way - you're losing a variable but adding a media query. This way is technically more flexible, too, not that we need it to be. Feel free to create an issue if you think we should remove it.
| @@ -224,12 +232,23 @@ | |||
| .pf-c-page__header-nav { | |||
| grid-column: 1 / -1; | |||
| grid-row: 2 / 3; | |||
| min-width: 0; | |||
There was a problem hiding this comment.
Is the min-width just for FF?
There was a problem hiding this comment.
no, it breaks in chrome without it, too. the default is min-width: min-content; so by default, the header-nav element doesn't overflow and just grows to fit it's content. That causes the nav and masthead to grow too wide, resulting in this brokenness.
| --pf-c-page__header-nav--lg--BackgroundColor: transparent; | ||
| --pf-c-page__header-nav--c-nav__scroll-button--nth-of-type-1--Left: calc(-1 * (var(--pf-global--spacer--md) - var(--pf-global--spacer--xs))); // This sets the the button to the edge of its container with 4px clearance on the left | ||
| --pf-c-page__header-nav--c-nav__scroll-button--nth-of-type-1--md--Left: calc(-1 * (var(--pf-global--spacer--md) - var(--pf-global--spacer--xs))); |
There was a problem hiding this comment.
Should we also do this to the 2nd button to make padding balanced on both sides
There was a problem hiding this comment.
To do that, if we move the button to the left a little, I think we'll need to add right padding to the horizontal nav so that the nav links don't show on the right side of the button (behind/underneath the button, since the nav extends to the right edge of the page). That would probably be a breaking change for folks who don't have the overflow buttons enabled, since their nav would no longer cut off/overflow to the edge of the browser and would cut off at the padding instead. But if we're OK with the way this looks for folks that don't have the overflow buttons enabled, I think that's a good change.
What do you think @kybaker, is this OK?
FWIW, this is what it looks like currently.
There was a problem hiding this comment.
@mcoker I do not think we want padding here. It looks like a bug and it is not as clear that there is overflow.
There was a problem hiding this comment.
@christiemolloy opened #1839
There was a problem hiding this comment.
Sounds good thanks @mcoker
|
🎉 This PR is included in version 2.6.15 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fixes #1794, fixes #1797