Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refactor nav block paddings/margins to inherit global styles. #31878

Merged
merged 6 commits into from
Jun 1, 2021
Merged

Refactor nav block paddings/margins to inherit global styles. #31878

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 May 17, 2021
ae60186
Fix color inheritance. Fix last-child margin.
jasmussen May 31, 2021
28b6c15
Use :where to minimize specificity.
jasmussen May 31, 2021
390ab32
Move chevron inside link.
jasmussen May 31, 2021
b60d6e1
Provide default paddings for submenus.
jasmussen May 31, 2021
8fd15ab
Fix page list bug.
jasmussen May 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 34 additions & 14 deletions packages/block-library/src/navigation/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,6 @@
stroke: currentColor;
}
}

// Set the default menu item padding to zero, to allow text-only buttons.
.wp-block-pages-list__item a,
.wp-block-navigation-link a { // This allows the custom menu item to inherit global styles.
padding: 0;
}
}


Expand Down Expand Up @@ -212,7 +206,12 @@
}
}

// Margins, paddings, and submenu positions.

/**
* Margins
* @todo: refactor this to use gap.
*/

// Menu items with no background.
.wp-block-page-list,
.wp-block-page-list > .wp-block-pages-list__item,
Expand All @@ -225,10 +224,11 @@
}
}

// When the menu has a background, items have paddings, reduce margins to compensate.
// Treat margins and paddings differently when the block has a background.
// @todo: refactor this to use gap.
.wp-block-navigation.has-background {
// 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-page-list,
.wp-block-page-list > .wp-block-pages-list__item,
.wp-block-navigation__container > .wp-block-navigation-link {
Expand All @@ -239,12 +239,32 @@
margin: 0;
}
}
}

a {
padding: 0.5em 1em;
}

/**
* Paddings
*/

// Set the default menu item padding to zero, to allow text-only buttons.
.wp-block-navigation a {
padding: 0;
}

// When the menu has a background, items have paddings, reduce margins to compensate.
// Treat margins and paddings differently when the block has a 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) a {
Copy link
Contributor Author

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:

.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:

Screenshot 2021-05-31 at 11 22 42

Copy link
Contributor Author

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

Copy link
Member

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 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/

Copy link
Member

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?

Copy link
Contributor Author

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.

padding: 0.5em 1em;
}


/**
* Justifications.
*/

// When justified space-between, open submenus leftward for last menu item.
// When justified right, open all submenus leftwards.
// This needs high specificity.
Expand Down