Navigation Block : try not using css variable for block gap support #37543
Conversation
glendaviesnz
requested review from
ellatrix,
oandregal,
spacedmonkey,
tellthemachines and
TimothyBJacobs
as code owners
glendaviesnz
changed the base branch from
trunk
to
try/no-css-variable-block-gap
|
Size Change: +37 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
glendaviesnz
changed the title
|
N.B. This approach doesn't currently work for sub-navigation links in mobile layout - they still get the theme default block gap applied instead of the custom block gap set on the block. There are probably a few other variations of the nav block that don't work. I won't spend any time trying to identify and fix these unless there is some agreement that this is a way forward. |
There was a problem hiding this comment.
Thanks exploring this approach @glendaviesnz!
There are probably a few other variations of the nav block that don't work. I won't spend any time trying to identify and fix these unless there is some agreement that this is a way forward.
I think another case is when a Navigation block chooses to list all pages, so the child of the Navigation block is a page list block. Its gap styling currently uses the CSS variable. But, I think for that block it might work to set its gap styling to inherit — then it should inherit the block gap setting of the parent Navigation block. I wonder this will work for some of the other edge cases? Here's a before / after of hacking in the inherit value in dev tools (apologise for the small screenshots, might need to expand / open in a new window to see):
| Before | After |
|---|---|
 |
 |
I like the approach of manually rendering the gap variable inline in the Navigation container, though, curious to hear what other folks think. If it winds up being viable, I imagine we'll then likely need to also handle grabbing the gap style for the block from the global styles settings, to support theme.json blockGap set at the block level. E.g. supporting the following:
youknowriad
force-pushed
the
try/no-css-variable-block-gap
branch
from
87c99bf to
58c63eb
Compare
ramonjd
force-pushed
the
try/no-css-variable-block-gap-navigation-block
branch
from
254f368 to
8ef4efc
Compare
I just tested using @andrewserong's suggestion and adding For example, .wp-block-navigation .wp-block-page-list, // page lists
.wp-block-navigation .wp-block-navigation__submenu-container, // required in order for the submenu to inherit
.wp-block-navigation .wp-block-navigation-submenu
{
gap: inherit;
}
There is additional top padding, courtesy of the existing rules that still use // Apply top padding to nested submenus.
.wp-block-navigation__submenu-container {
padding-top: var(--wp--style--block-gap, 2em);
}The assumption is that we'd remove, or at least make less specific, the current gap rules that define Maybe it's a good case for |
|
How should
play diff diff --git a/packages/block-library/src/navigation/style.scss b/packages/block-library/src/navigation/style.scss
index 26de41da03..3ad0e910c0 100644
--- a/packages/block-library/src/navigation/style.scss
+++ b/packages/block-library/src/navigation/style.scss
@@ -299,30 +299,39 @@ button.wp-block-navigation-item__content {
cursor: pointer;
}
-
/**
- * Margins
+ * Block spacing
*/
-// Menu items with no background.
-.wp-block-navigation,
.wp-block-navigation .wp-block-page-list,
-.wp-block-navigation__container,
-.wp-block-navigation__responsive-container-content {
- gap: var(--wp--style--block-gap, 2em);
+.wp-block-navigation .wp-block-navigation-submenu,
+.wp-block-navigation .wp-block-navigation__submenu-container {
+ gap: inherit;
}
-// Menu items with background.
-// We use :where to keep specificity minimal, yet still scope it to only the navigation block.
-// That way if padding is set in theme.json, it still wins.
-// https://css-tricks.com/almanac/selectors/w/where/
-.wp-block-navigation:where(.has-background) {
- &,
- .wp-block-navigation .wp-block-page-list,
- .wp-block-navigation__container {
- gap: var(--wp--style--block-gap, 0.5em);
- }
-}
+///**
+// * Margins
+// */
+//
+//// Menu items with no background.
+//.wp-block-navigation,
+//.wp-block-navigation .wp-block-page-list,
+//.wp-block-navigation__container,
+//.wp-block-navigation__responsive-container-content {
+// gap: var(--wp--style--block-gap, 2em);
+//}
+//
+//// Menu items with background.
+//// We use :where to keep specificity minimal, yet still scope it to only the navigation block.
+//// That way if padding is set in theme.json, it still wins.
+//// https://css-tricks.com/almanac/selectors/w/where/
+//.wp-block-navigation:where(.has-background) {
+// &,
+// .wp-block-navigation .wp-block-page-list,
+// .wp-block-navigation__container {
+// gap: var(--wp--style--block-gap, 0.5em);
+// }
+//}
/**
* Paddings
@@ -506,14 +515,14 @@ button.wp-block-navigation-item__content {
}
// Space unfolded items using gap and padding for submenus.
- .wp-block-navigation__submenu-container,
- .wp-block-navigation__container {
- gap: var(--wp--style--block-gap, 2em);
- }
+ //.wp-block-navigation__submenu-container,
+ //.wp-block-navigation__container {
+ // gap: var(--wp--style--block-gap, 2em);
+ //}
// Apply top padding to nested submenus.
.wp-block-navigation__submenu-container {
- padding-top: var(--wp--style--block-gap, 2em);
+ //padding-top: var(--wp--style--block-gap, 2em);
}
// A default padding is added to submenu items. It's not appropriate inside the modal.
|
@ramonjd, my understanding was that we just need to replace the instances where |
|
@talldan, @getdave - I think you both have spent a bit of time working on navigation block, so before we go any further with this PR, can you see any gotchas of taking this approach of manually setting the block styles gap value on the relevant wrapper and using There is also a question above that would be good to get some input on in relation to which levels of menu the block gap setting should be applied to. |
I think that's right. Sorry for the confusion. I only noticed that it was there and wanted to point out that it might contribute to whatever gap aesthetics we employ. For example, if we reduced the gap to |
|
One main consideration is that in general we should avoid setting presentational attributes on the block's items. This is because these are saved to the CPT and should be reusable across Themes and therefore should not contain presentation styles. Presentation should be stored on the parent Nav block instance and not on the inner blocks which are saved to the CPT. |
…default value for themes that may not have -wp--style--block-gap set
glendaviesnz
force-pushed
the
try/no-css-variable-block-gap-navigation-block
branch
from
7359f63 to
077a93b
Compare
…ad of applying style to innerblocks wrapper
|
@tellthemachines knows more than me about the styling aspects of the navigation block. @getdave makes a good point, that ideally the value for the gap comes from the navigation block itself. If the inner blocks inherit the value that sounds good. |
There was a problem hiding this comment.
This tests well for me @glendaviesnz 🎉
Tested in FF, Safari and Chrome.
I can see the gap value in the flex layout rules changing accordingly.
In the Block Editor my nav menus look like
In the frontend
Mobile view
With blockGap disabled in theme.json (removes all blockGap values)
In the Site Editor, the blockGap controls also work as expected.
There was a problem hiding this comment.
Thanks for putting this together!
The gap inheritance is working fine for most child blocks; the one slightly off scenario is when Social Icons is a child of Navigation. Social Icons still uses --wp--style--block-gap, and when a gap value is set in Navigation, that variable is generated in the front end but not in the editor, so Social Icons shows the the default value in the editor, but the Navigation value in the front end.
Social Icons has its own spacing settings so it can always override whatever it inherits, but would be good to have editor and front end matching. This can probably be fixed in another PR though.
Re submenus, given they render as dropdowns, they shouldn't need to follow the same spacing rules as the main Nav elements.
glendaviesnz
deleted the
try/no-css-variable-block-gap-navigation-block
branch
@tellthemachines I have noted this issue on another issue that is looking at related layout issues with the Social block, but will create a new issue if @andrewserong thinks it would be better dealt with separately to that. |
Description
Trying to refactor navigation block so gap is still supported when removal of CSS gap variable is merged
Because the elements that need
gapapplied are not the direct children of the block wrapper to which gap is applied after this refactor, this PR instead manually applies the value from theblockGapattribute to the relevant navigation item wrappers.How has this been tested?
blockGapintheme.jsonbehaves as expected and also setting a defaultblockGapvalue intheme.jsonScreenshots