-
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
Improvements to "inherit default layout" toggle #41893
Conversation
Size Change: +2.52 kB (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
Thank you for picking this up! I am confused about the text, should it not be the other way around? That the nested blocks fill the width of the container when the option is toggled off? |
Here's what I'm seeing: Definitely clearer than "inherit default layout"! "Blocks use full width" does make sense to me if I remember that we're referring to the "content" of the group block. I have the same question as @carolinan: should the help text be swapped around? I'm seeing this: |
Thanks for working on this one @tellthemachines!
I'm seeing the same, but I thought it was the right way around:
My understanding is that in this PR we're trying to communicate that when you insert a new Group block, it has a feature that the nested blocks will fill the current container, but that users can "switch off" this behaviour and use the default content width instead? (Or manually enter content / wide width) Is the confusion about what "full width" means? |
<p className="block-editor-hooks__layout-controls-helptext"> | ||
{ !! inherit | ||
? __( | ||
'Nested blocks use default content width with options for full and wide widths.' |
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.
Just double-checking: it isn't possible for blocks-based themes to switch off full
and wide
alignments altogether is it? I tested this PR in a classic theme without the alignments, and it results in showInheritToggle
being false, so I assume that if a user can see this control then their theme supports both full / wide options.
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.
Good question! Block themes can switch off wide/full alignments, but only by removing the layout
section in their theme.json settings altogether, which means they're not able to set content size/wide size either. I somehow don't see that being a popular choice, but if it is done, then the toggle doesn't show:
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.
Perfect 👍
I somehow don't see that being a popular choice
Me either, but good to know what happens just in case someone tries to get rid of it altogether!
I think the "options for full and wide widths" threw me since I associated them with the content and wide input fields. 🤷 |
Thanks for the feedback folks! The idea - which I took from this comment - is that when "Blocks use full width" is toggled on, the block doesn't use the theme content width, so the inner blocks fill the width of the container this option is set on. This is the same as having "Inherit default layout" toggled off. Inversely, when "Blocks use full width" is toggled off, the inner blocks use the theme content width (and the options for full/wide width also become available). Would it be clearer if we said "Inner blocks use full width" instead? And perhaps replaced "default content width" with "theme content width" in the description? |
I quite like that idea, personally! |
Ok, done! |
One other thing that's occurred to me: we could partially fix #33374 if we were to change the default on this setting to use theme content size on Group blocks at least. Worth doing as part of this PR? Or best addressed separately? |
Do you mean switching the default to use |
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.
This LGTM now @tellthemachines. Just left the tiniest of nits about adding a full-stop, but otherwise I think the language changes in this PR make the feature clearer to me 👍
Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com>
Thanks for the review @andrewserong !
Yes, though I think the only additional styles output are the max content width and auto margins applied to all the Group's children. I'll explore it separately, and definitely let's get #40875 ready first 😄 |
What?
Fixes #31950, taking into account suggestions in #36082.
Why?
The "Inherit default layout" toggle is confusing in how it's worded: what is the default layout? where are we inheriting it from?
How?
This PR tries changing the toggle text and adding descriptions to explain its actual effect on the block content.
Note
I'm not sure how we can best make the description text accessible. While it could be attached to the toggle field with an
aria-describedby
, the fact that the text changes according to the toggle position may make it confusing as a description. But on the other hand it is valuable to have the description be dynamic, so it explains what can be expected from the current state. Thoughts and suggestions welcome!Testing Instructions
Screenshots or screencast
Before:
After: