-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Only add layout classes to inner wrapper if block is a container. #48611
Conversation
Flaky tests detected in 217f4dc. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4300006453
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick fix, the explanation makes sense.
Testing notes:
- All deprecated gallery blocks are now showing the correct html markup (layout tag on parent)
- Current gallery blocks have correct mark up too -- tested columns, with a caption, standard
- Following current blocks also appeared to behave as expected
- Social Icons
- Query (various layouts)
- Buttons
- Group
- Column
- Navigation
- Comments (specifically checked pagination per shoutout in PR description)
I've approved the PR but would appreciate a logic check from someone who spends more time in the Gutenberg repo if possible.
Thanks for opening this one! I'll see if I can get a bit more info... Update: I was going way too quickly, I copied from the result instead of from the source when adding the test markup in my local environment 🤦 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, the logic looks good, and this is testing well for me with all the mentioned current core blocks, and the deprecated versions of the gallery block 👍
Thanks for the quick fix! ✨
@tellthemachines, do we need to backport this into the release branch? Based on the description, the fix is already in the core. |
@Mamaduka I believe there's a one-liner that appears to need to be backported — it's easy to miss, though, since it's a fix to the code that's in core, so I think this PR needed to migrate over the core code first before adding the fix. (Please correct me if I got that wrong @tellthemachines!)
|
Thanks for the reviews folks!
Yes that's correct. @Mamaduka it's probably a good idea to add it to the release branch in Gutenberg for consistency. I'm happy to prepare the core patch for it! |
Thank you, @tellthemachines! The core patch would be appreciated 🙇 |
I just cherry-picked this PR to the wp/6.2 branch to get it included in the next release: 0367c43 |
What?
Fixes #48606. While working on this change I noticed the updates to this logic from the Core backport code review hadn't been copied over yet, and as they are fairly minor and touch the same part of the code I'm also including them here.
The actual fix is just updating the
isset( $block['innerContent'][0] ) && 'string' === gettype( $block['innerContent'][0]
condition toisset( $block['innerContent'][0] ) && 'string' === gettype( $block['innerContent'][0] ) && count($block['innerContent']) > 1
because if the number of items in
$block['innerContent']
is less than or equal to 1, then either the block isn't a container for inner blocks, or it's a dynamic block such as Post Content or Navigation, in which case the logic to add classnames to the inner wrapper would have to be added in the block's custom PHP anyway.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast