Check child layout exists before generating classname. #61392
Conversation
tellthemachines
added
[Type] Enhancement
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
lib/block-supports/layout.php
Outdated
| // Prep the processor for modifying the block output. | ||
| $processor = new WP_HTML_Tag_Processor( $block_content ); | ||
|
|
||
| // Having no tags implies there are no tags onto which to add class names. | ||
| // Having no tags implies there are no tags onto which to add class names. |
There was a problem hiding this comment.
The whitespace here and below / before if ( ! $block_supports_layout && ! empty( $outer_class_names ) ) { appears to be off (extra indentation).
There was a problem hiding this comment.
Thanks for noticing! Not sure what happened here, the auto-formatter usually knows what it's doing 🤔
lib/block-supports/layout.php
Outdated
| * Add to the style engine store to enqueue and render layout styles. | ||
| * Return styles here just to check if any exist. | ||
| */ |
There was a problem hiding this comment.
Tiny nit: the indentation here appears to be one character off.
lib/block-supports/layout.php
Outdated
| * If columnSpan or columnStart is set, and the parent grid is responsive, i.e. if it has a minimumColumnWidth set, | ||
| * the columnSpan should be removed once the grid is smaller than the span, and columnStart should be removed | ||
| * once the grid has less columns than the start. | ||
| * If there's a minimumColumnWidth, the grid is responsive. But if the minimumColumnWidth value wasn't changed, it won't be set. | ||
| * In that case, if columnCount doesn't exist, we can assume that the grid is responsive. | ||
| */ |
There was a problem hiding this comment.
Indentation is one character off.
|
Thanks for the follow-up! Guarding behind a check for the child layout styles sounds like a good idea 👍 I haven't taken this for a spin yet, but plan to later on today. Just left a few little nits about whitespace indentation. |
|
I'm not sure how to test this manually. I think it's just my ignorance of how it's supposed to work 😄 I tried to follow the example in the test. Between trunk and this branch the frontend HTML looks the same to me: Paragraph has a colspan <div class="wp-block-group is-layout-grid wp-container-core-group-is-layout-1 wp-block-group-is-layout-grid">
<p class="wp-container-content-1">Test</p>
</div>Paragraph has no colspan <div class="wp-block-group is-layout-grid wp-container-core-group-is-layout-1 wp-block-group-is-layout-grid">
<p>Test</p>
</div> |
Manually you could compare the generated classnames in trunk and on this branch, for a page that has both layout and child layout blocks. In trunk, the child layout classname is generated for all blocks with layout, whether they have child layout or not. On this branch, it's only generated for blocks with child layout. So for the same markup, your generated classname on trunk might be |
There was a problem hiding this comment.
Thanks for the explanation, I can see the difference in the markup now:
Trunk: (the number of the content container is 3):
This PR: (the number for the content container is 7):
Layout and child layout rules appear to be working as on trunk as far as I can tell.
One of the other benefits of this PR IMO is that by nesting the child layout rules under the if statement, it's now clearer at a glance that all that code is just for the child layout rules, which improves readability to me 👍
LGTM!
Yeah I was considering splitting the style generation into a different function as a further improvement, but that can be for later 😅 Thanks for reviewing and testing! |
What?
In writing a test to accompany WordPress/wordpress-develop#6493, I realised the child layout classname was being generated for every block with layout, even if it doesn't have child layout. This PR adds the classname generation and remaining child layout logic inside a condition, so it only runs if there is child layout defined.
It also adds a test to check the child layout classname is generated.
I guess a further improvement could be to create a dedicated function for outputting child layout styles, similar to
gutenberg_get_layout_style, which would allow us to test the actual styles output, though that may have limited value in that it will produce more test strings that will have to be updated when child layout rules change.Testing Instructions
Run the PHP tests and make sure they pass:
npm run test:phpTesting Instructions for Keyboard
Screenshots or screencast