Layout: show inherit toggle in the absence of settings.layout object
#59580
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
…nherit toggle regardless of whether there is a `defaultThemeLayout` value. Presently, an empty object will pass the test so perhaps it's okay to remove the check? Also a bit of ESlint stuff for redundant var `css`
ramonjd
added
[Type] Bug
ramonjd
commented
Mar 5, 2024
There was a problem hiding this comment.
I'm not confident of this approach.
cc layout oracles @tellthemachines and @andrewserong
| @@ -125,14 +125,13 @@ export function useLayoutStyles( blockAttributes = {}, blockName, selector ) { | |||
| const fullLayoutType = getLayoutType( usedLayout?.type || 'default' ); | |||
| const [ blockGapSupport ] = useSettings( 'spacing.blockGap' ); | |||
| const hasBlockGapSupport = blockGapSupport !== null; | |||
| const css = fullLayoutType?.getLayoutStyle?.( { | |||
| return fullLayoutType?.getLayoutStyle?.( { | |||
| // and either the default / flow or the constrained layout type is in use, as the toggle switches from one to the other. | ||
| const showInheritToggle = !! ( | ||
| allowInheriting && | ||
| !! defaultThemeLayout && |
There was a problem hiding this comment.
So I don't really know if this is safe.
Things appear to work as expected.
The scenario is that:
allowInheritingistrue, butdefaultThemeLayouthas no value.
A value of {} would pass the test, so it seems safe to just wave it through, unless we specifically want to check for settings.layout.wideSize|contentSize?
Not sure 🤔
There was a problem hiding this comment.
Good question! I haven't taken this PR for a spin, but does this affect classic themes at all? And is it a problem if when you toggle the inherit toggle, if it doesn't immediately apply any rules? I.e. if folks go to set child blocks to "wide" versus content alignment they mightn't see any difference until a value is entered in for the constrained controls 🤔
These are just drive-by comments, though, thanks for working on the fix! If it's still open in a couple of days, happy to take it for a spin and give it a proper review 🙂
There was a problem hiding this comment.
I haven't taken this PR for a spin, but does this affect classic themes at all?
Good question. I smoke tested a couple and couldn't see any difference between this PR and trunk, but I didn't really go in depth.
I'm just wondering if the approach is any good right now 🤷🏻
And is it a problem if when you toggle the inherit toggle, if it doesn't immediately apply any rules? I.e. if folks go to set child blocks to "wide" versus content alignment they mightn't see any difference until a value is entered in for the constrained controls
I'm not sure - I can't see any issue, but I'm a bit layout blind.
Also, the fact that, on trunk,settings.layout can be an empty object and pass the test is a bit suspect.
If we tighten up that test, so that we need settings.layout to actually contain something, then we'd have to work out how to deal with the empty layout panel.
Whatever folks think is the best way forward...
There was a problem hiding this comment.
Given that this whole UI is going to change once #53455 (or something similar) is ready, it's not too important to make this a perfect fix.
is it a problem if when you toggle the inherit toggle, if it doesn't immediately apply any rules?
I think as long as the controls to set custom content/wide withs are shown, it's not super awkward 😅 because it exposes those controls, which once set will produce the visible effect.
We could perhaps refine the check to make sure that either the theme sets a contentSize or allowCustomContentAndWideSize is true, because if neither exist we might as well hide the whole panel.
|
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. |
|
Size Change: -17 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
tellthemachines
approved these changes
Mar 5, 2024
|
@ramonjd Friendly reminder to include the props in the commit description when merging. Also might be good to adapt the copy of the title (commit message). (For next times) |
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?
I'm not really sure if this is right, but this commit will show the layout inherit toggle regardless of whether there is a
defaultThemeLayoutvalue.Why?
Presently, an empty object will pass the test anyway, so perhaps it's okay to remove the check?
How?
Deleting the
defaultThemeLayoutcheck that's a condition for the toggle to display.Testing Instructions
In a block theme, remove the "layout" settings object:
{ "$schema": "https://schemas.wp.org/trunk/theme.json", "version": 2, "settings": { "appearanceTools": true } }Add a group block with "default" layout:
The inherit toggle should be present.
Screenshots or screencast