Style engine: remove enqueue flag
#43103
Conversation
…tyle engine should store CSS rules. Renaming $store_key to $store_name for consistency Updating tests and README.md Checking for valid $store_name in WP_Style_Engine_CSS_Rules_Store
ramonjd
added
[Type] Task
…yout.php because there's no selector and we dont want these styles to be stored anyway
There was a problem hiding this comment.
Thanks for the follow-up @ramonjd, the logic looks good to me here, now tying the enqueue logic to the presence of a context name 👍
✅ Elements and Layout styles are still output correctly in blocks-based themes
✅ Elements and Layout styles are still output correctly in classic themes (but output near the bottom of the page instead of in the head)
✅ With the removal of allow-listing the context, the added checks for a valid $store_name look good
Just added a couple of comments to make sure I'm understanding the intended changes correctly, but this LGTM! ✨
| // Block supports styles. | ||
| if ( 'block-supports' === $options['context'] ) { | ||
| $parsed_styles = WP_Style_Engine::parse_block_styles( $block_styles, $options ); | ||
| } |
There was a problem hiding this comment.
Just making sure I'm following along — we no longer need the block-supports check, because this function effectively doesn't do anything if it isn't calling WP_Style_Engine::parse_block_styles?
There was a problem hiding this comment.
Good question! That answer is "yes, and..."
I reasoned that wp_style_engine_get_styles encounters only Gutenberg style objects, which are:
- The value of
attributes.style(block supports) - In the future
i. the value of theme.json's rootstylesproperties
ii. the value of theme.json'sstyles.blocks[blockName]properties
iii. the value of theme.json'sstyles.elements[elementName]properties
These object share the same basic model, although there are some variations in property names and values.
For example, global styles will eventually pass custom CSS properties such as --wp--style--block-gap or dynamic references like { "ref": "styles.color.text" }. These, I think can be dealt with internally when the time comes.
Maybe.
😇
There was a problem hiding this comment.
All sounds good to me — and removing the check means it encourages us to think holistically about parse_block_styles for the global styles context in addition to the block-level, so I think this is a good direction, too 👍
| 'invalid_context' => array( | ||
| 'block_styles' => array( | ||
| 'color' => array( | ||
| 'text' => 'var:preset|color|sugar', | ||
| ), | ||
| 'spacing' => array( | ||
| 'padding' => '20000px', | ||
| ), | ||
| ), | ||
| 'options' => array( | ||
| 'convert_vars_to_classnames' => true, | ||
| 'context' => 'i-love-doughnuts', | ||
| ), | ||
| 'expected_output' => array(), | ||
| ), | ||
|
|
There was a problem hiding this comment.
Again, just confirming I'm following along: this test is no longer needed because we're no longer allow-listing the context? (We're effectively passing along the context as the store name, now, allowing any string-based value as the store name).
There was a problem hiding this comment.
Exactly! Thanks for clarifying.
What?
Follow up to #42880 (comment)
This PR:
enqueueflag in favour ofcontextto indicate that the style engine should store CSS rules.$store_nameinWP_Style_Engine_CSS_Rules_StoreWhy?
Since moving enqueuing to Gutenberg in #42880, we no longer need to tell the style engine to "enqueue" styles.
Testing Instructions
Test in both classic and block themes.
Create some content in a post with link colors and several "layout blocks", e.g., Group and Column.
Here's some test HTML:
Check that elements and layouts styles are enqueued.
Expected CSS output. For classic themes this style tag should appear at the bottom of the page. For block themes, in the head.
Run the tests!
npm run test:unit:php