Layout: Fix toggling off inner blocks content width setting for legacy markup #43888
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
andrewserong
added
[Type] Bug
|
Size Change: +2 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
glendaviesnz
reviewed
Sep 6, 2022
| @@ -172,7 +172,9 @@ function LayoutPanel( { setAttributes, attributes, name: blockName } ) { | |||
| layout: { | |||
| type: | |||
| layoutType?.name === | |||
| 'constrained' | |||
| 'constrained' || | |||
There was a problem hiding this comment.
Would a const legacyMarkup = !! inherit || !! contentSize be helpful to clarify why this is needed - could also be used in the checked prop as well?
There was a problem hiding this comment.
Oh, that's much nicer! I'll update, and we can use it in the check for help text, too 👍
andrewserong
force-pushed
the
fix/content-width-toggle-with-legacy-markup
branch
from
8173a69 to
bd3b1fd
Compare
glendaviesnz
approved these changes
Sep 7, 2022
andrewserong
force-pushed
the
fix/content-width-toggle-with-legacy-markup
branch
from
bd3b1fd to
ebff049
Compare
andrewserong
added
the
[Feature] Layout
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.
What?
In the Layout support's
Inner blocks use content widthtoggle, ensure that the logic for which layout type to set also factors in whetherinheritorcontentSizeis set for legacy markup that uses thedefaultlayout type.This ensures that for legacy markup (e.g. the Column block) if you go to toggle off the control, it will toggle off when first clicked.
Why?
Without this change, for some legacy markup, if you go to toggle off the control, it appears not to really do anything at first (it's actually updating the layout type from default to
constrained). To resolve this, in theonChangecallback, let's factor ininheritandcontentSizesettings in the same way as we do for the checked status.How?
Inner blocks use content widthtoggle'sonChangehandler, treat the existence ofinheritorcontentSizeas signalling that toggling should update the layout attribute to be an emptydefaultlayout object.Testing Instructions
Using the following legacy markup, go to toggle off the
Inner blocks use content widthcontrol on the right column:Test markup (the right column has content size)
Test that toggling the control still works as expected for existing Group, Query or individual Column blocks.
Screenshots or screencast
Note that in the before clip, the first click on the toggle clears out the content size, but does not visually set the toggle to false (this is because what's actually happening is that the layout object is being updated to be an empty constrained type). In the after clip, the logic is updated so that it treats the default layout type + contentSize as the same as a constrained layout type, so the toggle is effectively switched off when first clicked.