Skip to content
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

Update: Backport block settings to core. #4013

Conversation

jorgefilipecosta
Copy link
Member

This PR allows a block to define the preset settings of its nested blocks using the same shape as theme.json.
Backports WordPress/gutenberg#42124 and WordPress/gutenberg#46625 into the core.

Testing

  • With Gutenberg, disable paste the following markup on the block editor code editor and save the post.
<!-- wp:group {"settings":{"blocks":{"core/button":{"color":{"palette":{"custom":[{"slug":"button-red","color":"red","name":"button red"},{"slug":"button-blue","color":"blue","name":"button blue"}]}}}},"color":{"palette":{"custom":[{"slug":"global-aquamarine","color":"aquamarine","name":"Global aquamarine"},{"slug":"global-pink","color":"pink","name":"Global Pink"}]}}}} -->
<div class="wp-block-group"><!-- wp:paragraph -->
<p>Leaf paragraph of inner group block.</p>
<!-- /wp:paragraph -->

<!-- wp:buttons -->
<div class="wp-block-buttons"><!-- wp:button {"backgroundColor":"button-blue"} -->
<div class="wp-block-button"><a class="wp-block-button__link has-button-blue-background-color has-background wp-element-button">blue</a></div>
<!-- /wp:button -->

<!-- wp:button {"backgroundColor":"button-red"} -->
<div class="wp-block-button"><a class="wp-block-button__link has-button-red-background-color has-background wp-element-button">red</a></div>
<!-- /wp:button --></div>
<!-- /wp:buttons -->

<!-- wp:paragraph {"backgroundColor":"global-aquamarine"} -->
<p class="has-global-aquamarine-background-color has-background">global-aquamarine</p>
<!-- /wp:paragraph -->

<!-- wp:paragraph {"backgroundColor":"global-pink"} -->
<p class="has-global-pink-background-color has-background">global-pink</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->
  • Verify the result is the following on the post frontend (excluding the background color, which comes from my test theme):

Screenshot 2023-02-06 at 23 41 43

);

if ( ! empty( $styles ) ) {
wp_enqueue_block_support_styles( $styles );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method will be deprecated in favor of wp_style_engine_get_stylesheet_from_css_rules. See #4015.

Copy link
Member

@oandregal oandregal Feb 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested using the recommended method: it didn't work or I didn't know how to use it. It's best to look at this separately in a follow-up.

Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for preparing the backport.

Works as expected and code looks good (same as the Gutenberg PRs it backports).

@Mamaduka
Copy link
Member

Mamaduka commented Feb 7, 2023

PR also needs the core Trac ticket.

@jorgefilipecosta
Copy link
Member Author

Committed at daee074.

@andrewserong
Copy link
Contributor

andrewserong commented Feb 7, 2023

Just adding a follow-up comment regarding wp_enqueue_block_support_styles — apologies for missing the usage here! Over in Gutenberg, all other usage of wp_enqueue_block_support_styles had been removed and the function was marked for deprecation (with a corresponding core PR in #4015). The reasoning for the deprecation was that previously block supports output resulted in excessive style tags being output (because wp_enqueue_block_support_styles outputs one style tag per call). The alternative used by the other block supports is to use wp_style_engine_get_stylesheet_from_css_rules by passing it an array of CSS declarations paired with a selector (e.g. here's where the layout block support does it) — then, the style engine can gather together all the registered styles and output in a single <style> tag rather than the multiple tags output by wp_enqueue_block_support_styles.

In the case of this PR, it might not be a very simple switch, because the CSS rules aren't constructed within the block support, but are already generated by $theme_json_object->get_stylesheet(), so a refactor to the style engine might be a bit more complex to implement 🤔

In terms of next steps, I think it'd be good to:

  • Add tests for this block support. Without testing that the output from running this block support is correct, it would be easy for a PR like Deprecate wp_enqueue_block_support_styles #4015 to accidentally land.
  • Determine whether this block support needs to call wp_enqueue_block_support_styles by either:
    • Seeing if it's possible to get the CSS declarations and pass them to the style engine, instead of a rendered stylesheet or:
    • Extend the style engine to handle what is currently being generated by get_stylesheet (that's probably a bit of a rabbithole, so might not be feasible in the shorter-term)

Overall, I think deprecating wp_enqueue_block_support_styles isn't at all urgent, the task was originally added because it looked like all usage had been removed and @ramonjd and I missed this block support. (Apologies, again!) The main goal of the deprecation was to ensure that we avoid adding excessive <style> tags to the output on the site frontend where possible, and to leverage the style engine for grouping together styles for output. That's still a good goal to have, but the deprecation can happily wait for 6.3 if there isn't time to do it for 6.2.

TL;DR: It'd be good to add tests for this block support, but refactoring away the call to wp_enqueue_block_support_styles (and / or deprecating the function) can probably wait until 6.3 if need be.

@ramonjd
Copy link
Member

ramonjd commented Apr 5, 2023

The main goal of the deprecation was to ensure that we avoid adding excessive <style> tags to the output on the site frontend where possible, and to leverage the style engine for grouping together styles for output.

Thanks for looking into this @andrewserong

It looks like @aristath has already jumped down that particular rabbit hole 😄

https://github.com/WordPress/gutenberg/pull/48955/files#diff-fc395cfa5d5651167c668b9940c0fee8c346cca30d9e6087a2f2dcbe11f1e589

I hope to look at this PR very soon.

It would be good to deprecate wp_enqueue_block_support_styles as it's open to abuse - every time it's called it creates and appends a new style tag and was responsible for much of the HTML clutter from block supports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants