Try: Fix navigation gap & padding issues. #35752
Conversation
| @@ -231,7 +231,7 @@ | |||
| .wp-block-navigation, | |||
| .wp-block-navigation .wp-block-page-list, | |||
| .wp-block-navigation__container { | |||
| gap: calc(var(--wp--style--block-gap, 2em) / 4) var(--wp--style--block-gap, 2em); | |||
| gap: var(--wp--style--block-gap, 2em); | |||
There was a problem hiding this comment.
I'm aware this changes both the horizontal and vertical gap into a single gap property. But there didn't seem to be value in making this one be different.
| @@ -242,7 +242,7 @@ | |||
| &, | |||
| .wp-block-navigation .wp-block-page-list, | |||
| .wp-block-navigation__container { | |||
| gap: calc(var(--wp--style--block-gap, 0) / 4) var(--wp--style--block-gap, 0.5em); | |||
| gap: var(--wp--style--block-gap, 0.5em); | |||
There was a problem hiding this comment.
I'm not sure why the fallback didn't work for me, but you can test on trunk: use empty theme, which doesn't supply a block gap value, then add a background color to the navigation block and observe how items collapse. Removing the calc and simplifying the selector seems to fix it for me.
There was a problem hiding this comment.
This is strange. I'm not sure why either, but when I don't use the shorthand definition, it looks as expected.
row-gap: calc(var(--wp--style--block-gap, 0) / 4);
column-gap: var(--wp--style--block-gap, 0.5em);
With shorthand gap: var(--wp--style--block-gap, 0.5em);
Having said that, I think having consistent gap/column values makes sense and I don't see any strange UI issues with the extra bit of row gap.
👍
There was a problem hiding this comment.
Having said that, I think having consistent gap/column values makes sense and I don't see any strange UI issues with the extra bit of row gap.
Just an idle thought (and I'm sure there's a good reason not to do this), but I was wondering if since this block opts in to the line height support, whether we need to use row-gap? Could we set only column-gap and allow line-height to effectively control the vertical spacing between nav items?
There was a problem hiding this comment.
Might be worth an experiment, thanks for the suggestion @andrewserong.
The Navigation Block does already opt-in to line-height.
I think the difference would be that the submenu text would also feel the effects of any line-height changes.
Fooling around with vertical spacing via line-height:
With gap spacing only:
And I think line-height is rather meant to define a font/text property? I'm no expert though and may be interpreting the specs incorrectly.
There was a problem hiding this comment.
Just an idle thought (and I'm sure there's a good reason not to do this), but I was wondering if since this block opts in to the line height support, whether we need to use row-gap? Could we set only column-gap and allow line-height to effectively control the vertical spacing between nav items?
I like to think that line height and gap can coexist, but also that for spacing things out, gap will be more intuitive. I also still have hopes that we can have the user surfaced block spacing control toggle between unified/axial, at which point you can tweak if you need to.
| @@ -256,7 +256,7 @@ | |||
|
|
|||
| // When the menu has a background, items have paddings, reduce margins to compensate. | |||
| // Treat margins and paddings differently when the block has a background. | |||
| .wp-block-navigation:where(.has-background) a { | |||
| .wp-block-navigation:where(.has-background) .wp-block-navigation-item__content { | |||
There was a problem hiding this comment.
Possibly a recent regression as of https://github.com/WordPress/gutenberg/pull/35716/files#diff-d66a4ed73d0d50a799547b78db235a4bad3428a0decb8895752032cd849d01f0R254, or the followup in https://github.com/WordPress/gutenberg/pull/35725/files#diff-d66a4ed73d0d50a799547b78db235a4bad3428a0decb8895752032cd849d01f0R46, but using the generic class (which we should've been using anyway) fixes an issue where has-background menu items had zero padding, when they should in fact have had some.
| @@ -465,7 +465,7 @@ | |||
| } | |||
|
|
|||
| // A default padding is added to submenu items. It's not appropriate inside the modal. | |||
| & :where(.wp-block-navigation__submenu-container) a { | |||
| a { | |||
There was a problem hiding this comment.
Small fix for the overlay menu, to space things right.
|
Size Change: -81 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
This PR tests well for me. Thanks again for spotting the issue with the shorthand introduced in addede3. Possibly due to Or, and this seems plausible, because the computed value doesn't correspond to a valid A "length" is a number followed by a unit. So maybe: // This doesn't work because `4 * 4 = 8` has no unit.
gap: calc(4 * 4) var(--wp--style--block-gap, 0.5em);
// But this works because `4px * 4 = 8px` is calculated with a `px` unit.
gap: calc(4px * 4) var(--wp--style--block-gap, 0.5em);
Still, I think using the same value for row and columns makes things consistent and reduces a bit of the complexity. I don't see any UI issues with combining them. 🍺 |
|
Thanks for opening this PR @jasmussen! I left a comment on one of the threads about whether we could use only A possibly unrelated issue, I noticed that the background color behaves slightly strangely on TwentyTwentyOne (the classic theme) where the background color is applied to the nav items but the container is styled to pick up the global background color. In narrower instances of the navigation block, this results in an odd area of background color sneaking in:
I didn't quite get to the bottom of where the style rule is coming from, but I imagine it might be a theme issue? |
|
Thanks a bunch for the reviews.
Is your concern mostly about when navigation menus wrap? Right now submenus don't use the gap value:
I suppose we could make it do that. But can/should that be a separate PR? The TT1 issue also exist in trunk: And I believe it's related to this issue, for which there's a fix here. I'll rebase to see if that makes the RN test pass, but if things look good, I'd appreciate a green check! Thanks again. |
jasmussen
force-pushed
the
try/fix-nav-gap-issues
branch
from
4daa81c to
e3548db
Compare
This is indeed coming from the theme .wp-block-navigation .wp-block-navigation__container {
background: var(--global--color-background);
padding: 0;
}See: https://themes.trac.wordpress.org/browser/twentytwentyone/1.4/assets/css/style-editor.css#L1400 |
|
Good sleuthing! Setting the background color of the navigation block used to be a trick that enabled colorizing the dropdown menus, so they weren't white. This is handled more elegantly today in the block itself. |
And if we do, we can always add it back. Thanks! |
|
Thanks for following up on the TT1 issue, good to have that confirmed 👍
Yes, it was mostly about how they wrap, but I think in most cases the consistent gap value will likely still look pretty good, and we hope to come back to axial support eventually anyway. Nice work getting this fix in, Joen! |
I definitely would like to see support of row-gap and column-gap instead just one value. This could be optional and done like the dimensions/padding controls for buttons for instance. |
There's been some mild cogitating around this, nothing meaty yet. See: #35778 (comment) |
Description
Followup to #35277 (comment).
Fixes two issues:
Before:
After:
How has this been tested?
Here's some test content:
Test in a few themes, including a block theme that does provide a gap value, and a few themes that don't (such as tt1 blocks and Empty Theme), and test that nav menus with and without background colors all appear harmoniously spaced.
Checklist:
*.native.jsfiles for terms that need renaming or removal).