Add utility classnames back to blocks that have layout attributes specified #41487
Conversation
glendaviesnz
added
the
CSS Styling
|
Size Change: -577 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Nice work @glendaviesnz, thanks so much for honing in on this change, I think adding a separate function for generating the classnames is the right way to go — it makes it a bit cleaner than trying to do everything in the gutenberg_get_layout_style function 👍
I noticed that the e2es are failing due to the changed markup with the additional classes. In case it helps, in the other PR, I ran the following locally to update the snapshot:
npm run test-e2e:playwright -- -u test/e2e/specs/editor/various/post-editor-template-mode.spec.js
One thing to watch out for when updating the snapshot, though, is that the site title is different in CI than locally, so I had to manually change restore it in the txt file. Separately to this PR, we might be able to improve that test so that it's a little more resilient to layout markup changes 🙂
lib/block-supports/layout.php
Outdated
| @@ -190,12 +223,12 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) { | |||
| // If a block's block.json skips serialization for spacing or spacing.blockGap, | |||
| // don't apply the user-defined value to the styles. | |||
| $should_skip_gap_serialization = gutenberg_should_skip_block_supports_serialization( $block_type, 'spacing', 'blockGap' ); | |||
| $style = gutenberg_get_layout_style( ".$class_name", $used_layout, $has_block_gap_support, $gap_value, $should_skip_gap_serialization, $fallback_gap_value ); | |||
| $style = gutenberg_get_layout_style( ".$block_classname.$container_class", $used_layout, $has_block_gap_support, $gap_value, $should_skip_gap_serialization, $fallback_gap_value ); | |||
There was a problem hiding this comment.
As in the other comment, for this PR, we might not need to increase the specificity by prepending $block_classname (we could leave it for #40875)
| } | ||
|
|
||
| if ( attributes?.layout?.orientation ) { | ||
| layoutClassnames.push( `is-${ attributes.layout.orientation }` ); |
There was a problem hiding this comment.
@andrewserong is there a js equivalent of sanitize_title that we should use here?
There was a problem hiding this comment.
Other comparable files in this package use lodash's kebabCase. I usually try to avoid adding in lodash, but since this function is already used in the package and it seems like a straightforward way to do it, personally, I'd use that 🙂
|
I have updated the doc comment on the method to make it clear that this is an interim step to add back removed classes, rather than a place to start adding any new utility classes |
Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com>
Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com>
Strange, I didn't change these and now they are passing 🤔 |
ah, of course, removed |
Ah, yes, that makes sense now. Thanks for confirming! |
| { | ||
| [ `wp-container-${ id }` ]: shouldRenderLayoutStyles, | ||
| }, | ||
| getLayoutClasses( attributes ) |
There was a problem hiding this comment.
This might be slightly nit-picky, but in the PHP implementation, the classnames will only be added if the block has opted-in to the layout support. Theoretically in this JS version, it looks like if the attributes have a layout property (with the appropriate sub-properties), it'll output the classnames even if the block hasn't opted in to the layout support. I can only imagine this happening in an unfortunate edge case of a custom block in a plugin that happens to use a layout attribute.
Do you think it's worth adding a shouldRenderLayoutStyles check before calling getLayoutClasses?
There was a problem hiding this comment.
Good catch, thanks, will sort that out in the morning
There was a problem hiding this comment.
Thanks for this one @glendaviesnz, this is testing nicely for me:
✅ The specified classes based on layout attributes are output within the editor
✅ The specified classes based on layout attributes are output on the site frontend
✅ None of the added classes are serialised to post content
This LGTM, it adds back the classnames, which appears to be safe, while not making any changes to the layout support logic, which we'll be addressing in separate changes. Also, since the classnames aren't serialized, it should be pretty straightforward to make tweaks in follow-ups if need be. 🎉
glendaviesnz
added
the
Backport to WP Minor Release
|
This was seen as a regression by a number of people in 5.9, but not looked at in time for 6.0, so probably worth getting into 6.0.1 if we can. |
|
@mtias picking up on your comment here, The general view is that the style engine is a better place to handle the addition of these classes, but we are probably still a little way off getting everything in a place to make this possible. This PR handles the addition of the classes without serialising them back into the static block content, so migration from this temporary approach to the style engine when ready should be straightforward. Does this seem like a reasonable approach? cc/ @youknowriad |
…cified (#41487) Co-authored-by: Glen Davies <glen.davies@a8c.com> Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com> Co-authored-by: Cory Birdsong @cbirdsong
|
I just cherry-picked this PR to the wp/6.0 branch to get it included in the next release: f253a84 |
adamziel
removed
the
Backport to WP Minor Release
|
@glendaviesnz this PR contains PHP changes that need a matching PR in the https://github.com/WordPress/wordpress-develop repository to be included in WordPress 6.0.1. Would you please create such a PR? CC @SergeyBiryukov |
|
@adamziel WordPress/wordpress-develop#2840 is approved - do you need me to do anything else? I don't have any permissions on the wordpress-develop repo. |
|
Fantastic @glendaviesnz, thank you so much! We're good – I don't have these permissions either :-) But @SergeyBiryukov does – would you please merge that PR? |
…utes specified. [WordPress/gutenberg#38719 In 5.9 these utility classnames were removed], which removed the ability for theme/plugin authors to assign their own custom CSS related to specific layout selections. This was mostly related to the Button block. This commit adds these classes dynamically based on attributes, rather than saving them to the serialized content. Original PR from Gutenberg repository: * [WordPress/gutenberg#41487 #41487 Add utility classnames back to blocks that have layout attributes specified] Props glendaviesnz, peterwilsoncc, andrewserong, zieladam, matveb, samikeijonen. See #56058. git-svn-id: https://develop.svn.wordpress.org/trunk@53568 602fd350-edb4-49c9-b593-d223f7449a82
…utes specified. [WordPress/gutenberg#38719 In 5.9 these utility classnames were removed], which removed the ability for theme/plugin authors to assign their own custom CSS related to specific layout selections. This was mostly related to the Button block. This commit adds these classes dynamically based on attributes, rather than saving them to the serialized content. Original PR from Gutenberg repository: * [WordPress/gutenberg#41487 #41487 Add utility classnames back to blocks that have layout attributes specified] Props glendaviesnz, peterwilsoncc, andrewserong, zieladam, matveb, samikeijonen. See #56058. Built from https://develop.svn.wordpress.org/trunk@53568 git-svn-id: http://core.svn.wordpress.org/trunk@53157 1a063a9b-81f0-0310-95a4-ce76da25c4cd
…utes specified. [WordPress/gutenberg#38719 In 5.9 these utility classnames were removed], which removed the ability for theme/plugin authors to assign their own custom CSS related to specific layout selections. This was mostly related to the Button block. This commit adds these classes dynamically based on attributes, rather than saving them to the serialized content. Original PR from Gutenberg repository: * [WordPress/gutenberg#41487 #41487 Add utility classnames back to blocks that have layout attributes specified] Props glendaviesnz, peterwilsoncc, andrewserong, zieladam, matveb, samikeijonen. Merges [53568] to the 6.0 branch. See #56058. git-svn-id: https://develop.svn.wordpress.org/branches/6.0@53569 602fd350-edb4-49c9-b593-d223f7449a82
…utes specified. [WordPress/gutenberg#38719 In 5.9 these utility classnames were removed], which removed the ability for theme/plugin authors to assign their own custom CSS related to specific layout selections. This was mostly related to the Button block. This commit adds these classes dynamically based on attributes, rather than saving them to the serialized content. Original PR from Gutenberg repository: * [WordPress/gutenberg#41487 #41487 Add utility classnames back to blocks that have layout attributes specified] Props glendaviesnz, peterwilsoncc, andrewserong, zieladam, matveb, samikeijonen. Merges [53568] to the 6.0 branch. See #56058. Built from https://develop.svn.wordpress.org/branches/6.0@53569 git-svn-id: http://core.svn.wordpress.org/branches/6.0@53158 1a063a9b-81f0-0310-95a4-ce76da25c4cd
…utes specified. [WordPress/gutenberg#38719 In 5.9 these utility classnames were removed], which removed the ability for theme/plugin authors to assign their own custom CSS related to specific layout selections. This was mostly related to the Button block. This commit adds these classes dynamically based on attributes, rather than saving them to the serialized content. Original PR from Gutenberg repository: * [WordPress/gutenberg#41487 #41487 Add utility classnames back to blocks that have layout attributes specified] Props glendaviesnz, peterwilsoncc, andrewserong, zieladam, matveb, samikeijonen. Merges [53568] to the 6.0 branch. See #56058. Built from https://develop.svn.wordpress.org/branches/6.0@53569 git-svn-id: https://core.svn.wordpress.org/branches/6.0@53158 1a063a9b-81f0-0310-95a4-ce76da25c4cd
…utes specified. [WordPress/gutenberg#38719 In 5.9 these utility classnames were removed], which removed the ability for theme/plugin authors to assign their own custom CSS related to specific layout selections. This was mostly related to the Button block. This commit adds these classes dynamically based on attributes, rather than saving them to the serialized content. Original PR from Gutenberg repository: * [WordPress/gutenberg#41487 #41487 Add utility classnames back to blocks that have layout attributes specified] Props glendaviesnz, peterwilsoncc, andrewserong, zieladam, matveb, samikeijonen. See #56058. Built from https://develop.svn.wordpress.org/trunk@53568 git-svn-id: https://core.svn.wordpress.org/trunk@53157 1a063a9b-81f0-0310-95a4-ce76da25c4cd
|
In the 6.0.1 it seems that the alignment utility classes works only with the "wp_render_layout_support_flag" filter enabled, am I missing something? |
|
Hi @andrewserong and thanks for the clarification, I have just a couple of thoughts concerning this topic. The opt-out mechanism for custom a project has always be to dequeue the Block editor stylesheet, this way the logical classes are still applied and you can build your custom style based on these. This choice to remove them when you are still not ready to bring back a fallback is systematically breaking the style of all the installations that rely on this basic concept. I understand that this task is not easy and quick to implement, so for this reason all these experiments (like This new auto generation strategy for style is indeed more performant (with some drawbacks of course), as discussed in some Gutenberg threads here on Github, but should be considered in a more wide scope because for how this has been designed it will have more sense if the theme is entirely built with the Site editor, that is still a work in progress tool not suitable for clients projects. |
|
Thanks for the extra info @simom!
The way I imagine the flag might work is that by default, the classes would still be generated, and instead of dequeuing / overriding I'd be curious to hear if you have an idea in mind about how it should ideally work — because individual blocks (both in blocks-based and classic themes) require layout styles in order to be rendered correctly, I imagine both features (classes and styles) will be opt-out rather than opt-in. Feel free to comment here (or in a separate issue if you think it's a more unique case) if there's a specific way you think themes can ideally opt-in / opt-out. I'm following a few different discussions on the topic, and am keen to understand the use cases 🙂 |
|
Hi @andrewserong , thanks for your time, we can continue the discussion in this issue. As previously said this kind of behavior makes more sense in a broader context like a theme designed to fully embrace the site editor so should be enabled if the theme define a theme-json file. The layout fallback strategy should always be the block editor css file that still contains all the layouts logic. If we assume that this functionality should be turned on by default, an idea could be to have a filter to turned off globally or individually the style generation (bringing back the logical classes of course), something like: I don’t know if somebody else raised this topic but the main issue with this approach is that the styles defined by the blocks are questionable and subjective, some more, some less, and the most difficult to be used is the columns block (I’ve opened a discussion about it here #41984), and this approach is a little more “strong” than a classic stylesheet in my opinion. |
|
Excellent, thanks for the extra context @simom! I like the filter idea for disabling output, I'll play around with a couple of ideas and link a PR to this thread once I've got something working. And thanks for opening up the separate discussion for Columns — there are lots of issues to tease apart when it comes to layout, so good idea ensuring we've got that captured separately 👍 |
|
I've opened up a PR to explore a theme support feature to opt-out of all generated Layout styles in #42544 (the goal is to effectively do the same thing as the current approach that some folks use of overriding the Layout block support callback, which as of #40875 will no longer work to completely remove layout styles). Hopefully having it as a documented theme support will make it a bit clearer that base flex Layout styles are an opt-out feature, along with documenting that themes that do that will be responsible for providing structural layout styles for the blocks that depend on it. Happy for feedback over on the PR! |
|
Hi @andrewserong and thanks for your feedback! What will be the role of the block editor stylesheet in the future? If I add the theme support 'disable-layout-styles' the layout technically shouldn't still work because there is still the layout style in the main block-editor stylesheet? Obviously this is not something really useful because the stylesheet layout style should be the same of the auto generated one, but I'm just curious. |
mburridge
added
the
Needs Dev Note
|
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. |
…utes specified. [WordPress/gutenberg#38719 In 5.9 these utility classnames were removed], which removed the ability for theme/plugin authors to assign their own custom CSS related to specific layout selections. This was mostly related to the Button block. This commit adds these classes dynamically based on attributes, rather than saving them to the serialized content. Original PR from Gutenberg repository: * [WordPress/gutenberg#41487 #41487 Add utility classnames back to blocks that have layout attributes specified] Props glendaviesnz, peterwilsoncc, andrewserong, zieladam, matveb, samikeijonen. Merges [53568] to the 6.0 branch. See #56058. Built from https://develop.svn.wordpress.org/branches/6.0@53569
…utes specified. [WordPress/gutenberg#38719 In 5.9 these utility classnames were removed], which removed the ability for theme/plugin authors to assign their own custom CSS related to specific layout selections. This was mostly related to the Button block. This commit adds these classes dynamically based on attributes, rather than saving them to the serialized content. Original PR from Gutenberg repository: * [WordPress/gutenberg#41487 #41487 Add utility classnames back to blocks that have layout attributes specified] Props glendaviesnz, peterwilsoncc, andrewserong, zieladam, matveb, samikeijonen. See #56058. Built from https://develop.svn.wordpress.org/trunk@53568
…utes specified. [WordPress/gutenberg#38719 In 5.9 these utility classnames were removed], which removed the ability for theme/plugin authors to assign their own custom CSS related to specific layout selections. This was mostly related to the Button block. This commit adds these classes dynamically based on attributes, rather than saving them to the serialized content. Original PR from Gutenberg repository: * [WordPress/gutenberg#41487 #41487 Add utility classnames back to blocks that have layout attributes specified] Props glendaviesnz, peterwilsoncc, andrewserong, zieladam, matveb, samikeijonen. Merges [53568] to the 6.0 branch. See #56058. Built from https://develop.svn.wordpress.org/branches/6.0@53569 git-svn-id: http://core.svn.wordpress.org/branches/6.0@53158 1a063a9b-81f0-0310-95a4-ce76da25c4cd
What?
Add several layout utility classnames to blocks that have layout attributes specified
Why?
In 5.9 these utility classnames were removed which removed the ability for theme/plugin authors to assign their own custom CSS related to specific layout selections. This was mostly related to the Button block
How?
Adds these classes dynamically based on attributes, rather than saving them to the serialized content
Testing Instructions
is-content-justification-centerandis-nowrapclasses are added in editor and frontendScreenshots or screencast