Layout: only show inherit toggle in default layout (hide on Row block variation)#39665
Merged
andrewserong merged 1 commit intoMar 23, 2022
Conversation
andrewserong
added
[Type] Bug
andrewserong
commented
Mar 23, 2022
| const showInheritToggle = !! ( | ||
| allowInheriting && | ||
| !! defaultThemeLayout && | ||
| ( ! layout?.type || layout?.type === 'default' || layout?.inherit ) |
Contributor
Author
There was a problem hiding this comment.
Note: I've added layout?.inherit to this conditional so that any blocks that are currently in the weird state of having a flex type but inheriting from the default layout can switch off inherit via the UI.
Contributor
There was a problem hiding this comment.
Should we make the toggle disappear if they do toggle it off in this state or is that too confusing for the user?
Contributor
Author
There was a problem hiding this comment.
Thanks for taking a look @glendaviesnz — currently, toggling inherit also switches the type to default since the layout attribute is cleared out of anything but the inherit property (so the toggle would still be visible). Given how the toggle works, I'm hoping we don't actually have anyone in that weird circumstance of inherit being set to true and the type being set to something other than default.
I mostly added the layout?.inherit to be cautious just in case something was set manually to that state, but I'm not sure if it ever would have been possible to get into that state via the UI, so it's possibly an artificial example 🤔
|
Size Change: +47 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
Contributor
Right now, I think this is a safe assumption. |
youknowriad
approved these changes
Mar 23, 2022
youknowriad
left a comment
youknowriad
left a comment
Contributor
There was a problem hiding this comment.
This makes sense to me.
glendaviesnz
approved these changes
Mar 23, 2022
glendaviesnz
left a comment
glendaviesnz
left a comment
Contributor
There was a problem hiding this comment.
Worked as advertised for me
Contributor
Author
|
Thanks for the reviews, folks! 🙇 |
andrewserong
deleted the
update/layout-to-hide-inherit-toggle-on-non-flow-based-layouts
branch
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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?
Hide the "Inherit default layout" toggle if the current layout type is not of the default layout type. This allows us to hide the toggle in the Row variation of the Group block.
This is an opinionated change — the assumption here is that the default layout that we're inheriting from will always be implicitly of the "default" layout type. Is this a safe assumption to make?
Why?
As raised in reviews of #39650, the inherit toggle doesn't make much sense for a Row block, and possibly not for any other Flex-based layout. The toggle for the Row block also caused a problem that it'd unexpectedly switch the blog over to the Group variation again. Hiding the toggle altogether potentially resolves both problems.
How?
Update the conditional that determines whether or not to show the layout "Inherit default layout" control so that it can only appear on default / flow layouts.
Testing Instructions
To be really thorough, also check that you can switch off inherit on a block that does have inherit and "flex" so that these erroneous blocks don't wind up in a stuck state. E.g. using the following markup:
Screenshots or screencast
Note, the Orientation control is visible in the Row block in the below screenshots, but will be removed as of #39650.