Fix Buttons styling issue caused by Layout support being applied to blocks that have not opted in #34189
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…t have not opted in to the experimental layout
andrewserong
requested review from
spacedmonkey and
TimothyBJacobs
as code owners
andrewserong
added
the
[Type] Regression
12 tasks
|
Thanks for working on a quick fix @andrewserong 👍 I've given this a quick test. The buttons block aligns visually on the frontend now however it does not in the block editor. Should the same change be reflected in the layout supports JS hook? As to whether there is a better approach, I'm lacking the context around this work to be able to add much there so will have to defer to others. |
|
Thanks for testing @aaronrobertshaw, I've updated the layout supports JS hook to match the updated logic for the server-rendered support. I missed that in my haste to come up with a fix! 😀
Same here, I'm sure there was a good reason for including the check (I'm wondering if it was to reduce the number of times we're adding in the styles, so empty Group blocks wouldn't render unnecessary styling rules?), but I'm not too certain the specific reason from looking at the code change. Let's see what the others think, too 🙂 |
|
Size Change: -19 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
|
This change is actually intended. The reasoning is as follows:
For example you can test a Cover block here and you'll notice that the block gap is not applied properly anymore. Now the problem we have is that there are some blocks that should be defined as "flex" layout (Buttons, Navigation, Social Links) but since they were developed before, they just hacked the "flex" layout styles instead of actually making it a declarative layout. And third-party blocks might be doing the same which could cause regressions there. One potential solution is:
|
|
Thanks for the extra context @youknowriad, much appreciated. I see Nik rolled in the revert to #34194, so I'll close out this PR 🙂 |
youknowriad
deleted the
fix/layout-support-being-applied-to-buttons-block
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This is a follow on from #33812 to address a regression for the styling of the Buttons block. In that PR, the logic to skip the Layout support was updated to check that the Layout support isn't enabled and that the block doesn't have inner blocks. This means that blocks that haven't opted-in, but do have inner blocks will skip this check and have the container class applied, causing a styling issue for the Buttons block, which is laid out horizontally by default.
In this PR, I've reverted the check back to just checking if the Layout support is enabled (I tried switching it to an
||check, but that introduced further issues with the Group block not getting the styling it needed). There could be a better approach to this, but it seems to fix the issue so far CC: @youknowriadHow has this been tested?
Before applying this PR, in TT1-blocks theme, I added a Buttons block with multiple buttons to a post and viewed on the front end of my site. I noticed the styling regression in the screenshots below.
After applying this PR, the styling issue is no longer present, and the Buttons block no longer has the container class added.
Screenshots
Note that in the before image, the container class is added, and because the Buttons block hasn't opted in to the Layout, the default layout style gets applied (this CSS rule), which affects the top margin on all but the first Button block.
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).