Style Engine: block supports backend #39446
Conversation
ramonjd
added
Global Styles
ramonjd
force-pushed
the
try/style-engine-block-supports-backend
branch
from
50ed43b
to
3b1ed82
Compare
ramonjd
force-pushed
the
try/style-engine-block-supports-backend
branch
from
2aec1ea
to
1187a23
Compare
| */ | ||
| class WP_Style_Engine_Gutenberg_Test extends WP_UnitTestCase { | ||
| function test_returns_inline_styles_from_string() { | ||
| $style_engine = WP_Style_Engine_Gutenberg::get_instance(); |
There was a problem hiding this comment.
Does it need to be a class? Why not just a function, mostly curious about the reasoning here.
There was a problem hiding this comment.
Good question. It doesn't have to be a class. The reasons why I left it as a class are:
- because I copied the structure over from one of our initial experiments 😄
- to namespace methods and encapsulate constants such as
BLOCK_STYLE_DEFINITIONS_METADATA(similar toWP_Theme_JSON_Gutenberg) - portability across applications
- related to the last point, so we can be free to use generic method names such as
generateto harmonize the backend and frontend APIs, as mentioned below. - while a more conceptual point, to lend the functionality a degree of authority and conspicuousness, that is, so folks recognize it as a unified collection of properties and behaviours.
There was a problem hiding this comment.
Good question, and good answers! 😁
while a more conceptual point, to lend the functionality a degree of authority and conspicuousness, that is, so folks recognize it as a unified collection of properties and behaviours.
Yes, in PHP, because we're not using namespaces and can't encapsulate our functions in a separate module like we do in JS, I think it's better for us to gather together our style engine functions in a class rather than expose each of them in the global namespace. Plus, it's looking like we'll likely need to carry around some state in the style engine to deal with de-duping styles and / or deferring processing to a later part of the execution process, so starting with a class likely makes it easier for us to extend the style engine in follow-up PRs, too.
There was a problem hiding this comment.
Makes sense. I just wanted us to avoid the "stateful" trap if possible. When we add state to classes that are not meant to be classes, the problems start happening :)
|
@youknowriad Thanks for your comments and help on this one. I wanted to also get your opinion on:
|
ramonjd
force-pushed
the
try/style-engine-block-supports-backend
branch
from
2304cff
to
83e0bbd
Compare
There was a problem hiding this comment.
Apologies if this isn't ready for a full review. I'm just a bit late to the party and trying to get my head around all the style engine work.
I've tried testing this as per the PR instructions however it doesn't work for me.
Any margin styles added to the block are lost. In their place, I get duplicate inline padding styles. This occurred both with the example code and manually adding my own site title block.
| Trunk | This PR |
|---|---|
 |
 |
I've added a couple of questions as inline comments. Feel free to ignore them if I've just missed something obvious.
There was a problem hiding this comment.
Thanks for reworking this to be closer to the JS implementation, @ramonjd! I ran into similar issues to Aaron where the margin was rendered twice for the site title block:
In terms of the style engine being called multiple times, or processing things more times than it should:
- Do we try to ensure that we're only calling
generateonce per block instance (across all block supports), and then defer as much as possible to the style engine? Or: - Do we selectively call the style engine with only part of the
styleobject. For instance, in thespacingsupport, perhaps we'd only callgeneratewith an associative array containingspacingso that we don't accidentally process typography at the same time 🤔
I think I'm leaning toward (at this stage) calling the style engine only once in the spacing support, and processing the style object a little bit before sending it to the style engine. What do you think?
|
Thanks @aaronrobertshaw and @andrewserong for taking a look at this. Sorry for wasting your time with buggy code. I've pushed the code I should have pushed when I pushed this morning before I got distracted 🙏 |
ramonjd
force-pushed
the
try/style-engine-block-supports-backend
branch
from
22ea656
to
443dfc8
Compare
I agree that we should only call it once. ✅ It crossed my mind but I got bogged down thinking about how to determine the logic for I guess we could rebuild the style object before we pass it. Very rough (but working) example 😄 ...
$spacing_block_styles = array();
if ( $has_padding_support ) {
$padding_style_value = _wp_array_get( $block_styles, array( 'spacing', 'padding' ), null );
if ( $padding_style_value ) {
$spacing_block_styles['padding'] = $padding_style_value;
}
}
if ( $has_margin_support ) {
$margin_style_value = _wp_array_get( $block_styles, array( 'spacing', 'margin' ), null );
if ( $margin_style_value ) {
$spacing_block_styles['margin'] = $margin_style_value;
}
}
$inline_styles = $style_engine->generate(
array( 'spacing' => $spacing_block_styles ),
array(
'inline' => true,
)
);
...
if ( ! empty( $inline_styles ) ) {
$attributes['style'] = $inline_styles;
}
return $attributes;Or we could have In a later version I'd also like to be able to support passing a path like I was trying to keep it kinda the same as the JS version, but maybe we don't need to be so strict about that? What do you reckon? |
|
Thanks for following up, it's a tricky balance to strike!
I like that idea, I think it's worth exploring further. In terms of responsibilities, I think it makes sense that "block supports" owns whether or not something is supported, and the "style engine" owns generating the styles, so this kind of logic feels like a good direction to me.
Nice — yes, iteratively moving toward being able to call the style engine as few times as possible sounds good, and this sort of thing could mean calling once in
I don't think we need to be too strict, personally — as we implement things in PHP or in JS we'll encounter things that reveal changes we need to make in the other one. Here we've encountered two things, I think, that warrant going in a slightly different direction to what we expected if we need to:
We'll then hopefully learn from the PHP implementation what's missing from the JS side and vice versa 😀 |
I don't mind if it's done separately in another PR but I'd love if we can do soon to avoid falling in the trap of having two different JS/php implementations. Keeping them together kind of gives the necessary incentive that they should be implemented the same.
Yes, I think so. The difficulty is going to be more on the "naming" as we need to write the package as if it was consumed by core (so no gutenberg_ prefixes) but we also need to have a dedicated version in gutenberg that don't conflict with core (so gutenberg_ prefixes). The way we do this for the |
When you put it like that it sounds so obvious! 😄 Good call.
Excellent. This is great to know. And thanks for the interesting tips. I'll take a closer look at the block-library. 🙇 |
|
Size Change: -170 kB (-14%) 👏 Total Size: 1.04 MB
ℹ️ View Unchanged
|
I rolled it back already. Although the build step is part of the PHP tests (and I assumed the webpack copy), they still fail for some reason. I'll throw up a separate PR for moving the style engine PHP file to the package folder so we can get some expert 👀 on it. 👍 |
ramonjd
force-pushed
the
try/style-engine-block-supports-backend
branch
from
f96bba4
to
42a12da
Compare
There was a problem hiding this comment.
Thanks for all the follow-ups, @ramonjd!
Just gave this a re-test and it's still working nicely for me, and I tested the tests locally to make sure that they all break in expected ways if we fiddle with the expected data 👍
This LGTM as a place for us to start now, particularly with the intended follow-up PR to look at the build process and switching out of the Gutenberg suffix in the classname.
Just left a couple of tiny comments about comments, but nothing major. Would it be good to get a second approval since I'm maybe slightly biased toward the implementation? 😁
… a Gutenberg style attributes object
Adding base test file
Add optional options array Explicitly tell `generate()` to return styles so we can open it up to further functionality Update tests
Update comments, fix typos Filter CSS output
…st be resolved in another PR.
Co-authored-by: Andrew Serong <[email protected]>
ramonjd
force-pushed
the
try/style-engine-block-supports-backend
branch
from
4ace927
to
bf9f444
Compare
|
Given that I've sneaked |
Tracking Issue: #38167
What?
A backend version of
Experimenting with classname obfuscation.Reverted (see commit history)Works on block support styles (margin/padding for now) that are serverside rendered, e.g.,
core/site-titleWhy?
To create a backend engine for style object -> css rule transformation to complement the frontend code: #37978
How?
WP_Style_Engine_Gutenberghas aSTYLE_DEFINITIONS_METADATAdictionary that contains instructions on how to parse a Gutenberg style attributes in order to return CSS rules.Testing Instructions
Insert a few
core/site-titleblocks and add padding.Editor code
Check the resulting output and CSS inline styles.
Related PRs