-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Style engine: elements backend support #40987
Conversation
|
||
// Enqueue block support styles where there is a selector. | ||
if ( $should_enqueue_block_support_styles ) { | ||
if ( isset( $this->registered_block_support_styles[ $selector ] ) ) { |
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.
What do you reckon about registering and enqueuing block styles @andrewserong ?
I know you tried something similar over in #38974
Too much too soon?
My idea was that we could leverage registration for all block styles, mainly layouts, but also consolidating elements as they're added.
cc @youknowriad
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.
Too much too soon?
I think this PR is probably a good time for us to start exploring it, given that this is the first block support we're dealing with refactoring that depends on rendering in a separate style
tag.
I know you tried something similar over
The thing I hadn't looked at yet was how will we want to call the style engine from the theme JSON class? Since enqueueing / outputting is already happening over there, I wasn't sure if we'd move that to the style engine, too, eventually, or if we'll leave that class to handle it. In which case, I guess it'd be calling the style engine without the enqueue_block_support_styles
option, so it's probably fine?
My idea was that we could leverage registration for all block styles, mainly layouts, but also consolidating elements as they're added.
That sounds good to me — with the slight caveat that we'd ideally be moving a bunch of layout styles to be generated via presets (as in #39708). So, I imagine ideally via the style engine, we'll be mostly handling real values that need to be generated rather than common styles for output? Or were you imagining we'd deal with those here, too?
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.
The thing I hadn't looked at yet was how will we want to call the style engine from the theme JSON class?
Thanks for the comment!
I'm playing (only playing) around with that over in #40955
Basically hotswapping static::to_ruleset( $selector, $declarations );
for $styles = gutenberg_style_engine_generate( $node, array( 'selector' => $selector ) );
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'm playing (only playing) around with that over in #40955
Nice one! Well, that seems to confirm that guarding the behaviour behind the $should_enqueue_block_support_styles
option here means that the implementation in global styles output can happily co-exist with enqueuing for when individual block supports are rendered. 👍
Just an idle thought, but in terms of naming I wonder if we can remove the block_support
from the option name and just call it enqueue_styles
since theoretically we could one day use the enqueuing outside of block supports (if it winds up being better to use the enqueuing for the global styles output, too, for example)?
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.
if we can remove the block_support from the option name and just call it enqueue_styles since theoretically we could one day use the enqueuing outside of block supports
Terrific question.
I agree. Ultimately we could (and probably should?) work towards rendering global styles in the same block, so the name wouldn't be appropriate.
I did also wonder about this at the time. The reason I kept the block_supports
in the name for now was because there appears to be a deliberate distinction between global styles and block styles in https://github.com/WordPress/gutenberg/blob/trunk/lib/compat/wordpress-5.9/script-loader.php#L63
So I stole borrowed the code from gutenberg_enqueue_block_support_styles()
, which doesn't do half the work of gutenberg_enqueue_global_styles_assets()
So I suppose it was mainly to remind me not to load anything else without checking first 😄 Plus I reasoned it was a private method and therefore "renameable".
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.
there appears to be a deliberate distinction between global styles and block styles
Oh, great point (and about it mostly being contained in private method names, aside from the $options
param). I think given that we're still in "refactor working features" phase of the style engine work, keeping the consistency with the existing behaviour there sounds like a good way to do it.
And like you say, if and when we get to consolidating with global styles, we can combine the two as needed when we get there 👍
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.
Do you think enqueue_block_support_styles
should be an option of the "generate" function or it should be something separate? Why did you choose this pattern over something like (in the block support code)
$styles = gutenberg_style_engine_generate( $style_object );
register_block_styles_support( $styles );
I think this change of block styles output (moving from directly rendered styles to "registered styles" is a good thing but I also expect it to have subtle (order and things like that) impacts we might miss originally, so we need to be careful here.
Also, I'm curious how this (the move to registered styles) could help address the issues we have right now where calling get_the_content
/ REST API don't return the styles attached to a patch.
How could we leverage that later to have maybe a new property in the content.styles
in the REST API or something like that. Also, in the PHP API, how can we do something like $output = get_content( $post )
and output would be something like an array with both styles and markup (and JS later?). I guess the question is how to formalize the output of a post with all the potential assets dependencies
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.
and JS later?
Definitely 🙂
If we ever want to add client-side transitions, we need to know the CSS+JS requirements for each block, whether those are external assets (URLs), their own script
/style
tags or references to other (more global) script
/style
tags. And the same is true for the REST API (the famous issue), and could be a way to ease the integration of Gutenberg on headless sites.
5a20a41
to
7fa7be9
Compare
Yes, very good question. Thank you! 😄 It was nothing but an attempt to consolidate all the elements link styles within a single style tag instead of Furthermore, I thought elements was a good place to try it out, given that elements is limited to one style (link colors) at the moment. I'm still of the opinion that we should be registering such block support styles eventually, especially when we start looking at the layout area of the plugin, not only to have them rendered in one place, but to have the opportunity to process them. By "process" I mean the potential for deduping, ordering, generating helper classnames. None of that is on the table right now of course, but we'd need to register styles in order to do that. Nevertheless, I thought previously that it could be "too much too soon", and now I think about it, it does go against the principle of not changing what we output in phase 1. I wrote about that in the tracking issue update:
So now I'm reflecting on that statement, perhaps it's better to remove style registration from this commit with the lesson that it "works" until we need it next time? What do you think?
@andrewserong and I briefly touched upon this topic over at: The conclusion was that it's tricky and we don't know right now. We have a bucket of styles, but we need to think about a way to retrieve those styles for any given block container based on its markup, HTML attributes or 🤷
We threw a couple of crazy idea about, e.g., pulling styles from the style engine registry based on unique classnames or other attributes of a block container. If there were no rules in life we could experiment with generating a unique id for every registered block "style" and add a @andrewserong summed up the challenge well by saying "what's the simplest way we could retrieve the unique styles that are generated by the style engine, for an arbitrary piece of post content. Hopefully, via presets, we limit the volume of these styles that we need to deal with in this way, but there'll still be some" |
That's a great question, thanks for raising it!
I think that's a good potential argument for keeping the the option in the call to the style engine, rather than taking the output and calling So, it'll be interesting for us to look at the shape of data we'd like have returned from an API endpoint when content is fetched. And then, what a plugin or other code needs to do with that data to ensure that all relevant styles are enqueued, presets are available, and styles specific to the rendered blocks are output on the page. There's a fair few moving pieces!
I quite like this idea, or the idea of a data structure in the response that enables the styles to be reconstructed. It could be useful to have more than just direct CSS, though, as there could still wind up being some double-ups?. But, ideally, once we've moved the Layout support to generate its common styles via presets, the styles in the API response should 🤞 just be those values that are unique to the set of rendered blocks, so maybe that isn't so much of an issue.
I think that'd be a fair call for this PR, if it simplifies things. There's so many different directions we could go in implementing style registration and output that it's good for us to divide up the problems where we can, if it helps unblock things. |
Thanks folks for the discussion, to clarify I'm on board with the idea of "registering output styles" somehow and do optimizations/manipulations on them before output. What I'm uncertain about is whether we need to conflate these two things in the same "function":
It can be in the "style engine" bucket/package but I see them as two separate unit testable things.
I don't have a strong opinion here. It's true that changing how styles are output can be impactful and that we need to be very careful there. |
Thanks for the helpful feedback @andrewserong and @youknowriad
I think this is the logical way forward. With that in mind I don't have much hesitation to remove it from this PR, and creating a new one with that architecture in mind.
Totally. If we get elements in without registration we'll at least have something to test when we add it in, albeit using a separate and testable function/class! 🍺 |
f4b90a1
to
0cf1aa4
Compare
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.
This is testing nicely for me @ramonjd, thanks for honing in on a good change for this.
✅ Works nicely for blocks like Group and Paragraph that inline other block supports (e.g. Typography)
✅ Works nicely with server rendered blocks like Site Title
✅ Both custom colors and preset colors work correctly
Just left a few nitpicky comments (mostly comments and a little naming) but no real blockers. I think overall this change looks good to go to me! 🎉
Trying to come up with a model for elements
…bject from the style.elements array to the style engine since that's what it expects anyway
…cifies that they want to return classnames from preset values, the style engine will do that instead of building css var style values.
…n a single style tag. The idea is to extend this to layout as well.
This means the style engine will always attempt to output classes by default using the incoming `var:preset|*` values. The consumer is free to ignore them. The style engine will skip over `var:preset|*` values when building CSS rules. To return CSS vars in the CSS rules however, the consumer will need to specify a `css_vars` flag. The style engine will try to return `var(--preset--*)` values from `var:preset|*` values when building CSS rules. This gives us a way to return both the classname and CSS var if we wish, rather than a CSS var OR a classname.
…eparate that out into another PR>
Fixing typos Renaming variables for clarity Props @andrewserong
0cf1aa4
to
c33a765
Compare
Added the Needs Dev Note label in case this needs a dev note (either individual or as part of a "misc" dev note) for WP 6.1 release. |
Tracking issue: #38167
What?
color: var(--wp--css--heavenly-blue )
from'var:preset|color|heavenly-blue'
.selector { ...rules }
.This enables us to support element styles, which are printed to the page inside a style tag.
For example by passing in the
{ color: { text: 'value' } }
of{ elements: { link: color: { text: 'value' } } }
After all this, we can print out the backend styles for elements (just links for now).
Why?
Elements block supports requires us to generated CSS var values, e.g.,
var(--wp--preset--*)
fromvar:preset|*
presets.Furthermore, we need to print the styles into the page via
enqueue_block_support_styles
.Testing Instructions
Elements only supports link colors at this stage. So, add a few links with different preset and custom colors applied and make sure they look dandy on the frontend.
Example block code
Run
npm run test-unit-php /var/www/html/wp-content/plugins/gutenberg/packages/style-engine/phpunit/class-wp-style-engine-test.php