Refactor nav block paddings/margins to inherit global styles. #31878
Conversation
jasmussen
added
[Block] Navigation
jasmussen
requested review from
georgeh,
scruffian,
vcanales,
gwwar,
MaggieCabrera and
carolinan
|
Size Change: -666 B (0%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
|
I tested different padding combinations in theme.json with emptytheme. The page list is a separate block, but when I tried to style it with theme.json, the CSS output broke. |
|
Thank you for testing! I would agree about expected behavior, and the separate issue of Page List. I ticketed #31879 for that one. |
jasmussen
force-pushed
the
fix/nav-specificity
branch
3 times, most recently
from
6adafc7
to
15b1288
Compare
|
Rebased this one. Although there are some big changes here, as noted, they are important. Moreover, this should be easy to test insofar as trunk and this branch should look and behave exactly the same. |
jasmussen
force-pushed
the
fix/nav-specificity
branch
from
15b1288
to
68c06cf
Compare
jasmussen
force-pushed
the
fix/nav-specificity
branch
from
05e4f9c
to
28b6c15
Compare
| // 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) a { |
There was a problem hiding this comment.
@nosolosw I wanted to make sure you saw this, as I think it's a trick we should use a lot with global styles, now that we don't have to support IE11.
Here's the situation. We want to provide a default menu item padding for menu items. Zero padding when there's no background, a relaxed padding when there is. We can do that like so:
.wp-block-navigation a {
padding: 0;
}
.wp-block-navigation.has-background a {
padding: 0.5em 1em;
}
Those are fine as default paddings. However, and as is the purpose of this PR, global styles aims to let you customize these paddings, and it outputs them, for example, like so:
.wp-block-navigation-link a {
padding-top: 1.5em;
padding-right: 2em;
padding-bottom: 1.5em;
padding-left: 2em;
}
Unfortunately, in the battle against .wp-block-navigation.has-background a, it will lose.
However using the where selector, it still wins:
There was a problem hiding this comment.
I created a simplified codepen to outline the method: https://codepen.io/joen/pen/abJVrGX?editors=1100
There was a problem hiding this comment.
Oh, this is a very nice trick, thanks for sharing!
Thought a bit about how we could use this in the global stylesheet. Essentially, we output 1) block styles as block_selector { ... }, 2) blocks with elements, such as block_selector element_selector { ... }, 3) preset classes .has-value-preset_name { ... }, and 4) preset classes specific to a block, as in block_selector.has-value-preset_name { ... }.
This trick seems to be more appropriate for styles in the lower layers, such as this PR: styles that come with the block, whether it's added by core or a plugin. Is this your assessment as well? Do you see more opportunities to use this pseudo-class?
There was a problem hiding this comment.
This trick seems to be more appropriate for styles in the lower layers, such as this PR: styles that come with the block, whether it's added by core or a plugin. Is this your assessment as well?
Exactly. I recall our weapons-race eventually leading us to add !important modifiers to many global styles. While I can understand the potential necessity of that, I've been looking at ways to de-escalate that. And essentially, if we can reduce the specificity of all blocks and hopefully provide a pattern for themes to follow, that might let us reduce the specificity needed by global styles, and find a more harmonious balance. This codepen shows a basic example of a direction that could take:
html :where(.has-red-background-color) {
background-color: maroon;
}
html :where(.has-green-background-color) {
background-color: darkgreen;
}
These are default colors which, due to the where-selector has low specificity. In fact the specificity is so low that these classes, regardless of order in the stylesheet, will override them:
.has-red-background-color {
background-color: red;
}
.has-green-background-color {
background-color: green;
}
We might even increase the specificity of global styles in a different way, using the :is selector: https://css-tricks.com/almanac/selectors/i/is/
There was a problem hiding this comment.
I find the use of !important in user-generated styles (preset classes, the only place we use them) still necessary and the rationale holds for me (user styles have higher priority than any theme or core style). However, I've been hearing in some places that allowing to hook into these user styles may be necessary in some cases (dark mode, media queries). What would you think of #32627?
There was a problem hiding this comment.
Thanks for your thoughts here. I think I like that PR, but I need to think about it even more — CSS variables are all the rage, and I think we may be over-using them in some cases. But yes, that would be a way to add theme control 👍 👍
The reason I worry about the use of !important is that it's nuclear, it's the last resort, and there are almost always situations where using it means some legitimate use case becomes moot. But using it with precision as we do now, and in very specific cases, that might be fine. And if we find out in the future that it isn't, we have some ideas on what we can try instead.
|
In 390ab32 I moved the dropdown indicator inside the
Compare that to this:
As far as I can tell, this change does not appear to cause any deprecations nor does it appear to cause any other headaches with regards to appearance. Furthermore I don't see any accessibility problems with moving this as it already appears to have measures in place to hide it from screenreaders. Nevertheless I wanted to surface the change. |
|
Alright, I think this is ready for another review. It's refactored, and more heavily tested. I'll update the description with new testing content and more test cases. |
There was a problem hiding this comment.
I did a few tests and this seems to be working fine both in the editor and the frontend.
The code is sensible and I didn't find any issues.
As a side-effect, this also fixes issues when an outline is used on focus. For example in my theme I've got this:
a:focus {
outline: 2px solid;
outline-offset: 2px;
}Before:
After:
I liked having the caret outside the link... It meant that the door is open to implementing on-click behaviors for menus (as opposed to the current on-hover, which I passionately dislike). However, this does fix an issue we currently have, so IMO we can merge it and see about on-click when the time is right.
|
Thanks a ton for the review. I'm going to leave this one open for a bit still, to see if other folks have time to test and or share thoughts, and then depending on feedback I'll land this one tomorrow. It will enable some great followups I look forward to starting on.
Yes, I've been thinking a lot about how to best enable that in the future, and discussed it a bit in this thread. The short of it is that we'd probably either need to make it a split button, or a single toggle button anyway, needing to swap out some markup. So I think the split button is still possible if that's what we end up having to go with. Thanks again! |
|
I'm going to land this and will as always follow up on anything that goes wrong. |
Description
Fixes #31784.
This PR refactors how margins and paddings are applied to menu items, to allow it to inherit those values from what is defined in global styles. That requires reducing the specificity a great deal, as global styles simply targets a block name and then the link element inside.
The steps to test this are best described in #31784, but please try adding this to your experimental-theme.json:
By changing those values, menu items including submenu items should have padding adjusted to fix. In these screenshots I set it to the following:
Note that this change will cause a regression in the TT1 theme. Previously we increased specificity to override, but we can't do that and still inherit global styles. Which TT1 must be fixed separately. Specificaly, it adds padding to the wrong element:
Additionally, this change won't affect the Page List block. The Page List by having its own different CSS classes does not inherit any of the paddings that the global styles system outputs. Probably a separate issue, but nevertheless.
How has this been tested?
Updated May 31st.
I've refactored this again to fix some issues with display on the frontend. The ideal test matrix remains:
Intended behavior with no values in theme.json:
Intende behavior with spacing values defined in theme.json:
Please test using "Empty Theme" from https://github.com/WordPress/theme-experiments. As noted some themes provide their own paddings for the nav menu, meaning they will override our efforts here. More details in this trac ticket.
Test content
Here is some testing content you can use:
Types of changes
Because of the need to be able to inherit properties generated by global styles, I had to remove quite a few classes and specificity. While the overall structure is the same, this on its own can cause headaches, notably for themes. All that being said, I think this PR is worth it. I will create separate tickets for page list and TT1 to fix there, but the reduction in specificity is almost always a good thing, and giving control of padding directly to the global style system feels worth it.
Checklist:
*.native.jsfiles for terms that need renaming or removal).