-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor nav block paddings/margins to inherit global styles. #31878
Merged
Merged
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
68c06cf
Refactor nav block paddings/margins to inherit global styles.
jasmussen ae60186
Fix color inheritance. Fix last-child margin.
jasmussen 28b6c15
Use :where to minimize specificity.
jasmussen 390ab32
Move chevron inside link.
jasmussen b60d6e1
Provide default paddings for submenus.
jasmussen 8fd15ab
Fix page list bug.
jasmussen File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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:
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:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a simplified codepen to outline the method: https://codepen.io/joen/pen/abJVrGX?editors=1100
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 asblock_selector element_selector { ... }
, 3) preset classes.has-value-preset_name { ... }
, and 4) preset classes specific to a block, as inblock_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.