-
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
Global Styles: Add CSS vars for root level settings #29714
Conversation
Size Change: 0 B Total Size: 1.31 MB ℹ️ View Unchanged
|
👋 I've looked a bit into this and wanted to share some feedback. For context, we've tried this approach at the beginning of global styles (see #22296 for a recap) and the issues boiled down to:
Did you try to implement this with the style system (block supports / theme.json)? I'd be curious how far you can get. I can foresee some issues (the current block uses more colors than we have available via block supports). |
I don't see that this would be a problem. A block can choose to not specify any colors in which case it would simply inherit from its context. Alternatively it can implement its own colors as the calendar block currently does. This change opens up another option which I think would be very welcome to block developers - it gives blocks a way to know what the default colors of the site are and implement them in a flexible way.
This would change the variable only in the scope defined by this selector.
The advantage of the proposal is particularly for third party block developers - giving them a way to make their blocks look like the theme. |
I think the calendar block should have its own color and style options, like the table block. |
This is interesting proposal :) So like mentionned by @nosolosw is to that we initially started like that for Global styles: 1- Generate a CSS variable for each style config We quickly realized that it doesn't work though because of the CSS variable inheritence: when you apply a background to a container, all blocks inside it that support this variable (think buttons, tables...) get their background change in addition to their container, which is not the expected result. This is why we changed the approach, we decided to avoid step 1 and 2 above and just do the step 3 which is: generate the styles directly based on the config. This approach proved to work in a lot of situations just fine. But what you're showing in this PR that there might be still value in the original approach if it's only "opt-in" per blocks and not the only way to consume global styles. What you're doing in this PR is: For each global style config, generate two things: an actual style (like color: red) and a CSS variable definition (--my-var: red). In some situtations, the CSS variable is used, in others we just rely on the actual style and in others, we need both (containers like the root block or any group block come to my mind when I think about blocks that need both definitions) So this PR is impactful, it changes Global styles conceptually a bit by introducing "two modes" of application of global styles. I'm not against it personally but we'd have to think it properly:
I'd love more thoughts from @mtias @mcsf and if we ever we decide to go into this direction, we'd have to make sure to clarify the behavior properly in our docs... |
I find the most important part of this PR to be that we are giving both plugin and theme authors access to the variables that otherwise they didn't have access to (compared to how they can access others such as blocks') that are very important for the theme since they are on the root level. |
I left my brain to think about this while sleeping to see how we could address the issues above. Didn't have a lot of time to sum up my thoughts, so my braindump is a bit wordy. TLDR ― I think the core idea has the potential to enable something we aren't able to provide at the moment: maintaining contextual design links, which touches on things we were talking about at #27233 Let's say we have a calendar as a top-level block (within "root": {
"text": "var(--wp--preset--color--red)"
},
"core/group": {
"text": "var(--wp--preset--color--blue)"
},
"core/calendar": {
"background": "var(--wp--preset--color--red)"
} At the moment:
As Riad suggested, what if all containers (or blocks that opt-in via an API) output both styles and CSS vars. theme.json input:
The CSS output will be: :root {
color: var(--wp--preset--color--red);
--wp--style--color--text: var(--wp--preset--color--red);
}
.wp-block-group {
color: var(--wp--preset--color--blue);
--wp--style--color--text: var(--wp--preset--color--blue);
}
.wp-block-calendar{
color: var(--wp--style--color--text); // NOTE THIS VARIABLE.
} With this CSS we can make both 1 and 3.1 work because the value of the variable used by the calendar depends on the environment (root, group, etc) but it's agnostic as to which environment it lives in. We can do this with one variable per each style property we expose via theme.json. If we wanted all properties, this would be the whole list: --wp--style--border--color
--wp--style--border--radius
--wp--style--border--style
--wp--style--border--width
--wp--style--color--text
--wp--style--color--background
--wp--style--color--link
--wp--style--color--gradients
--wp--style--spacing--padding
--wp--style--typography--font-size
--wp--style--typography--font-family
--wp--style--typography--font-weight
--wp--style--typography--line-height
--wp--style--typography--text-decoration
--wp--style--typography--text-transform However, we still need to make 3.2 work as well. To make it backward-compatible, we need an approach that doesn't serialize anything to the DOM because we can't rewrite old posts. What if we use the preset classes to set the variables as well? Example: What we do now: .has-red-color {
color: red;
}
// At the moment, we only enqueue this if the block
// defines a preset different than the defaults.
.wp-block-columns.has-red-color {
color: #c6352e;
} What we could do: // Color is set for any block with the class.
.has-red-color {
color: red;
}
// Variables are set only for "containers"
.wp-block-group.has-red-color,
.wp-block-columns.has-red-color {
--wp--style--color--text: red;
}
// We still do this if the block
// defines its own presets.
// It'll overrride the above's ruleset
// if it's present.
.wp-block-columns.has-red-color {
color: #c6352e;
--wp--style--color--text: #c6352e;
} This would work for 3.2, but not fully: what if the user selects for the container a custom color (which doesn't attach the preset classes to the block but uses inline styles instead). For new blocks, we could add the CSS variable as inline styles for the container, but this won't work to address old posts. So, this is how far I've gotten to develop this idea. The issue I am not sure how to address is custom colors set at the block level for old posts. But the rest seems that it could work really nicely and reduces the need for "nested contexts" we were talking about in #27233 Perhaps we only need to be able to add template parts to the equation. I see a couple of ways to do it, but don't want to divert the conversation from this. |
edited my comment ☝️ to fix a couple of typos |
#29891 may be relevant to this discussion. In particular, as per design mockups, there's going to be a background element blocks can tap into. The specific implementation of elements (chaining selectors, CSS Custom Properties, etc) is going to be defined as we implement each. |
@scruffian @nosolosw I've been thinking about this PR again and I think we should do it, but I also think that we should be doing it with the following rules:
In this case, we should check the config of the block itself:
generate the style according to the config.
Reasoning
I'd love your thoughts on that @mtias @mcsf @scruffian @nosolosw @ockham @gziolo |
When talking privately with @nosolosw I asked if we could use CSS variables instead of "skipSerialization" flag and he mentioned some blockers. If what you are proposing resolves them, then I'm all for the proposed approach. Otherwise, we will be forced to find a way to pass all the generated styles and class names to the element that isn't a wrapper. That would mean also that developers would have to explicitly use some new API method that is responsible for that. If CSS variables is an option then we should make sure it is used as the API you outlined is far simpler than the amount of work to understand and properly apply "skipSerialization". |
I agree this is much simpler than skipSerialization. It seems to give a lot of power while remaining very simple. |
Apologies if this is obvious (I'm still trying to wrap my head around a number of these concepts), but what happens if the value is |
Just the regular global styles behavior, so in this particular case it results in |
I've been staring at this for a while, and I'm afraid I still don't really grasp the proposal 😅 One thing I've noticed is that this allows for a rather large number of different combinations of Would it be possible to list all different combinations, and spell out what the "global" generated CSS, and block attributes/markup would look like? I'm afraid I can't really weigh in on this otherwise. One off-the-cuff question would be: If this affects block attributes/markup, won't it lead to deprecations upon theme switch? |
@ockham would this comment help with the understanding of the proposal here #26752 (comment) |
I've spent some time thinking about this PR and the proposals above more and I came up to the conclusion that if we do it, it will suffer from the same issues that forced us to drop the CSS variables behavior of global styles in the first place. Let me clarify: Imagine there's a block (say table) that relies on the background color and apply it to its cells like so:
And in theme.json we define a background color in the root
What happens here is that we'll have:
In other words, this proposal only doesn't work for properties that doesn't cascade and only works for the ones that cascade (text color) and even for these, why not just rely on the regular cascade. So I'm leaning towards avoiding this proposal entirely at least for v1 of Global styles. |
16a5e15
to
b6997f4
Compare
@youknowriad I'm not exactly sure why you think this is a bad idea, but I've removed the change to the calendar block in case that was your concern. Please could you give more details about why you don't like this approach? From a themes perspective it will be very useful to have access to colors like the background that the user has set, as we might want to use it in places that won't cascade naturally. |
I tried explaining here #29714 (comment) in general trying to benefit from a cascade like behavior for things that don't cascade can cause unexpected results. Would you mind giving an example? |
Sure. So lets say I want to style table blocks in my theme, and I want the header of the table to contrast with the body like this: If I use CSS to achieve this, then if the user changes their background color to black using Global Styles, the header of the table won't be high contrast like this anymore. However if we had these values as CSS variables then I could use those variables in my CSS. Then when the user changed their colors the table would look like this: Does that help? |
@scruffian what happens if the table is inside a group block and that the user changes the background and text color for that particular group? |
@scruffian I don't understand why it inherits just the top level background/text colors? Imagine it's the sidebar where we invert the colors or there's a frame around the content where we invert the colors? |
Ok understood, so we're saying this:
unlike my example here #29714 (comment) you're not using the "background color" as a background but more as a "text color". It does seem very weird to me in the first place because why would something change for me as a user when setting a "background color" to a specific area. That said, maybe we can be liberal here and accept that themes can do this (not blocks). The issue becomes that we'll be generating a lot of variables assignments, we can try it though and see. cc @nosolosw @mtias @mcsf Also, how do we know which blocks generate these variables and where will we do it? |
I think that setting variables for root will be useful. I'm less sure that we need to do the same for every container. It's probably sufficient most of the time to simply have the root variables. |
Closing in favour of #39432 |
Description
We need a way for blocks to implement the default colors set in root. Often this happens via CSS inheritance but sometimes we want to use the theme colors in a block in a different way, which won't work with inheritance.
This PRs adds CSS variables for root level settings from theme.json:
This gives blocks a standard way of accessing the default colors for a site which is independent of the theme.
This PR was co-created with @MaggieCabrera
How has this been tested?
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: