-
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
Expose presets declared via add_theme_support in global styles #22076
Conversation
Size Change: +491 B (0%) Total Size: 822 kB
ℹ️ View Unchanged
|
How does this compare to @jorgefilipecosta's PR where he tries to use CSS vars for these presets too? |
You mean #21490, right? This PR is complementary in that it addresses the global level while 21490 works at the local level. It's also an enabler for 21490 as it may unblock some things that prevent 21490 from landing. I meant to comment there but it'll have to wait a few hours until I'm back from lunch. |
I've just stumbled upon #20588 and come to realize the whole role of With this newly gained knowledge, I'm thinking what prefix should we use for the presets: I may want to propose that these are taken off from the |
@nosolosw Here's the last proposal in terms of theme.json format #21583 (comment)
Can you clarify this? |
I'm still reading up other PRs that landed recently, but I was thinking of something along the lines of this for the
The reason is that presets should both configure the editor and generate CSS declarations (using custom or normal properties, that's not important atm). |
Thinking out loud a bit: another thought that I have is whether the top-level "global", "blocks" and "presets" should coalesce under a single key called "styles" (which is what they do => generate styles). Example:
If this made sense, the prefix for the generated variable could be actually none (what Jorge used in #21490): |
I don't think "palettes" or "presets" should be merged with "styles" because. The styles do generate CSS but the presets don't, the presets just generates CSS variables definitions which if not applied to any other style/selector won't do anything. For me "presets" are more "config" of the editor/frontend so they are more related to enabling/disabling things. Also noting that passing an empty palette actually disables the UI in the editor. All these three categories: "styles", "config" and "presets" can be "global" or "per block". That's the thinking behind the proposal. I can see how there's some similarity between "palettes" and "styles" but I think this technical similarity is more an "implementation detail" and not a conceptual one. (one configures the editor and one actually applies a style). -- There's also a third category I called "ui" on that proposal which is in fact the UI exposed on the global styles panel. |
I see how that logic also makes sense, I can be on board with that as well. So, to move this PR forward: would it be a fair assessment to say that we can go with the |
yes, that prefix is good for me 👍 |
3cf7e98
to
953c3d2
Compare
OK, pushed the changes here and to the theme for testing and also updated the issue description. This is ready for review. |
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.
question for later: I wonder if we should have a "php script" that generates the stylesheet and gets enqueued into the editor and frontend for these.
Can you expand on this? At first sight, I'd think global-styles.php is such a script, although it also does other things (register the CPT for user data, etc). |
i mean a php script that returns CSS, not something loaaded by other php scripts that render HTML. basically something you could use this way:
|
Hi @youknowriad, @nosolosw, Having a PHP script to load the global styles presents a big advantage we may allow browsers to cache global styles related to CSS variables, instead of sending them on every page load as inline styles. But there is a significant disadvantage for first-time visitors, or when the cache is invalidated, we may need to load the WordPress engine two times. The first time to generate the HTML and the second time to create global style variables. Users can customize them, so we need to query the database if we use the standard WordPress functions we instantiate another WordPress engine), I guess loading the engine two times has some impact. |
There's no need to load the whole WordPress "engine" on that script, we only need to load theme.json and any function/file used to generate the CSS. |
gutenberg_experimental_global_styles_get_from_file( | ||
locate_template( 'experimental-theme.json' ) | ||
), | ||
$theme_supports |
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 guess $theme_supports should be before gutenberg_experimental_global_styles_get_from_file to give more priority to theme.json in case it uses theme supports and theme.json at the same time.
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 was intentional, I tried to capture why in the comment before the return statement: essentially, there's yet a few options to store the presets in the theme.json and I didn't want people to start using something like this in theme.json
:
{
// global styles stuff
"presets": { /* this data would be processed */ }
}
Which brings me to a different question: at the moment, we don't validate that the keys in theme.json
are valid or follow the schema we expect (global, blocks, etc), so anything that theme authors put there will be converted to variables as well. I think this is fine for this stage and I even add some validation in some of the PRs I have somewhere. However, I'm on the fence if this is a good or a bad thing: in a way, having that freedom could enable theme authors to use it creatively (custom CSS). Do you have any thoughts about that?
Coincidentally, a few days ago, I've just stumbled upon an experiment by Jorge about a similar problem: how to make the editor-styles a stylesheet so it's cacheable. In that case, we couldn't do it because of how some hosts rearrange and bundle the CSS stylesheets to serve them from CDNs. I was wondering whether that's something that will be applicable to the generated CSS from theme.json as well. If we start adding more rulesets with selectors that target blocks we may need adding the editor wrapper as well (hence, we're in the same position and can't add this as a stylesheet). Another thought that I had was about hosts rewriting or not allowing certain URL paths to be reached (for example, |
@nosolosw WordPress already does that though |
Initial docs for this (and related PRs) at #22518 |
This PR exposes theme supported style presets (colors, gradients, font-sizes) declared via
add_theme_support
(see) in global styles so they can be used by themes directly in thetheme.json
.Example
Input from
theme.json
:Input from declared theme support via
functions.php
:Output:
Considerations
Each preset value is converted to a CSS custom property whose prefix is
--wp--preset--[feature]--[slug]
. Examples:--wp--preset--color-primary
,--wp--preset--font-size--small
, etc.In the future, we may be able to declare these presets directly in the
theme.json
. While we get there, this brings us a ready-to-use solution that theme authors can count on. It may be the case that, even if we are able to declare presets viatheme.json
, we may want to support the previous configuration mechanism.Testing
global-styles-inline-css
has the proper CSS custom properties (if the testing theme was used, it should have the same contents as in the example section above).