Try "constrained" content width as new layout type #42763
Conversation
|
Size Change: +621 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Fascinating idea @tellthemachines!
To make sure I'm following along, the idea with the default layout then, is that it's a simpler flow-based container that can group blocks and provide vertical spacing and alignments, but doesn't provide content size settings? Thinking about the available layout types, I find the idea of default/flow being quite simple and leaving things like content size to other layout types quite compelling!
I left a comment on layout.php, but I'm wondering what happens to Group blocks that are out in the wild (particularly older post/page content that is updated infrequently) that currently set content / wide size or set inherit to true. If we need duplicate handling for those cases, then I could imagine things starting to get messy again, where the shape of this PR is looking quite neat so far 🤔
In terms of naming, I think back when the Stack group block variation was added, folks avoided using Column as the name, to try to avoid confusion with the Column block. That said, I quite like the idea of a Column layout type, particularly if we eventually support settings like flex-basis for max-width, or effectively the kinds of features that are used in the Column block!
| @@ -43,6 +43,16 @@ function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support | |||
|
|
|||
| $style = ''; | |||
| if ( 'default' === $layout_type ) { | |||
There was a problem hiding this comment.
What happens to existing Group blocks with the default layout type, that set contentSize or wideSize?
Yes, that's the idea!
We should be able to handle those with a deprecation, migrating blocks with inherit to the column layout type. I'm not sure what the approach should be for third party blocks using layout though. Are there even any of those? And is it ok to have a breaking change if the layout support is still experimental? |
tellthemachines
force-pushed
the
try/new-layout-type
branch
from
c331ba8
to
b0a05b4
Compare
Thanks for confirming!
Unfortunately, I don't think a deprecation will be enough in this case as it requires the blocks to be edited in the editor before the rendering fix will take effect. This could be a tricky situation where we need non-migrated markup to still be rendered correctly 🤔
I'm not sure, but the layout support doesn't appear to be intended for third party blocks at this stage, as it's pretty much undocumented?
That's a great question. From my perspective, I think breaking changes in API-terms are probably fine, but breaking styles on existing sites, or requiring block markup to be migrated in order to render correctly is probably a blocker. It's definitely one of the harder areas for me conceptually with the Layout block support — because it's used in production on such heavily used blocks (like Group and Columns) it's very difficult to make significant changes without breaking something 😅 |
Hrm... unless if we infer the type based on the presence of the |
|
Yeah, I was thinking the styles to display content width only need the |
|
I updated with a deprecation for the Group block, as well as some server-side logic to handle cases where there are
So you did have a good point above @andrewserong 😅 I'm not sure how best to fix this. We could, of course, not change the default on the Group block and instead do something like I tried in #42582 to update new blocks on the fly. I still don't love that solution though 😅 |
Ah, yes, that's a good point. Thanks for digging in here! At the very least, it's really helpful to have these PRs that demonstrate a potential solution and the exact problems we encounter when attempting to implement it. It sounds like we've got a few trade-offs to weigh up here!
In some ways it feels like one of the challenges we face is a similar one to some other issues/PRs of wanting to be able to perform an action when a block is inserted, and only on insert. I wonder if there's any way for us to tease apart the difference between the default value (in terms of what's a default value when the block is serialised) from the value that is set when a block is inserted? That might be a bit of an API rabbithole to figure out, though 🤔 From my perspective, between the approaches, it feels like #42582 gets to the heart of the particular task at hand, but like you mention, that solution has its drawbacks too. I wonder if there's anything we can do with block variations, or use update logic that only applies if the Group block is empty? At the very least, that might help us hone in on only making changes to blocks that aren't doing anything visually on the front end of the site? Not sure how helpful that is as an idea, but I'm happy to do a bit of digging / hacking around this week, too! |
Not sure how helpful it is, but I had a go at this in #42876 @tellthemachines — it's still a pretty hacky approach, but tries to see if we can get it kinda working without deprecations. I was mostly curious to see if restricting the logic to only apply to empty Group blocks that haven't yet defined |
|
Thanks for helping me think this through @andrewserong !
I don't think there is at the point that matters to us here, which is in
I think attributes are our best bet to tell apart blocks with the legacy default layout from newly-added blocks that are meant to have the column layout by default, because they are the only info we have about each specific block. Unless I'm missing something, everything else we can access here - block defaults, layout definitions - concerns the block type or the layout type, so gives us nothing on the block itself. The thing is we can't add attributes to existing blocks unless they are being edited and can go through the deprecation, but we can add attributes to all new blocks as they are generated, which is what #42582 attempts to do. In which case, we don't need to change the block layout type, just change the edit logic to switch all new blocks to the column type as they're created.
I don't think adding a new variation would help us here, because we'd still face the problem of old default vs new default. And looking at Group content is risky because an empty Group might be used for formal purposes, as in this artwork! The more I think of it, the more I'm convinced that we need to do something like #42582. But I think what we're doing in this PR is also important, because it provides a way of configuring new block types to have content width from scratch. However, the logic from #42582 will need to change somewhat to accommodate this PR. I think I'll try bringing it in here, thus ending up with the mega-PR I was trying to avoid 😅 |
Hey, that's also a fantastic pattern to use as a test case to make sure we don't break anything! What a fun one 😍 And I see what you mean, from that pattern the following still renders on the frontend: This is output on the site frontend as:
If we were going with an empty / has no inner blocks check, then it'd probably also need to check that there are no attributes set at all 🤔... ah, it sure is tricky! 😅
So, basically, yes, I think you've hit the nail on the head here 👍 |
tellthemachines
force-pushed
the
try/new-layout-type
branch
from
9204ce2
to
ef50b1c
Compare
tellthemachines
requested review from
spacedmonkey,
ntwb,
nerrad,
ajitbohra,
ellatrix,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
There was a problem hiding this comment.
This is now ready for review! Don't be discouraged by the big diff, most of it is snapshot updates 😄
This PR does two main things:
- Introduces a new layout type to deal with content/wide widths and their associated styles.
- Sets some logic in the Group block so newly-added Groups get content width by default.
These two things could be done in separate PRs, but this way we can get a complete picture of how this works as a fix for #33374.
lib/block-supports/layout.php
Outdated
| ); | ||
| } | ||
| } | ||
| } elseif ( 'column' === $layout_type ) { |
There was a problem hiding this comment.
This is a very weird diff but essentially, what I did here was move all the contentSize/wideSize logic into a new "column" layout type
| // Set the correct layout type for blocks using legacy content width. | ||
| if ( isset( $used_layout['inherit'] ) && $used_layout['inherit'] || isset( $used_layout['contentSize'] ) && $used_layout['contentSize'] ) { | ||
| $used_layout['type'] = 'column'; | ||
| } |
There was a problem hiding this comment.
Blocks with contentSize or inherit are migrated to "column" layout on the front end.
| $wide_size = isset( $settings['layout']['wideSize'] ) ? $settings['layout']['wideSize'] : $settings['layout']['contentSize']; | ||
| $wide_size = static::is_safe_css_declaration( 'max-width', $wide_size ) ? $wide_size : 'initial'; | ||
| $block_rules .= '--wp--style--global--content-size: ' . $content_size . ';'; | ||
| $block_rules .= '--wp--style--global--wide-size: ' . $wide_size . ';'; |
There was a problem hiding this comment.
contentSize and wideSize values are added to root as custom properties. This is necessary so we can use them in the base styles declared in theme.json.
| @@ -46,7 +46,7 @@ export default function useAvailableAlignments( controls = DEFAULT_CONTROLS ) { | |||
| } | |||
|
|
|||
| // Starting here, it's the fallback for themes not supporting the layout config. | |||
| if ( layoutType.name !== 'default' ) { | |||
| if ( layoutType.name !== 'default' && layoutType.name !== 'column' ) { | |||
There was a problem hiding this comment.
We should in theory have been able to change this to if ( layoutType.name !== 'column' ), but that caused the BlockAlignmentUI unit tests to fail, because rendering that component is dependent on a layout type (which defaults to default) and in order for tests to succeed it needs to have some alignment controls inside it 😅
| name: 'core/paragraph', | ||
| attributes: { content: 'P' }, | ||
| name: 'core/button', | ||
| attributes: { text: 'Click' }, |
There was a problem hiding this comment.
So this is an interesting one. Ostensibly these tests are checking whether cut/paste actions work correctly for nested blocks. But they are also checking if the blocks are replaced more than once during paste. This last part started failing with this changeset, probably because the Group block now updates itself to use the column layout. So to preserve the original intent of the check, I've replaced the Group/Paragraph with Buttons/Button.
There was a problem hiding this comment.
I love what you're doing with this PR @tellthemachines! The ah-ha moment for me was looking at the logic in the UI for editing what used to be the inherit setting in the group block.
With this PR applied the logic now appears to be:
- Toggle
Inner blocks respect content widthoff results indefaultlayout being applied - Toggle
Inner blocks respect content widthtotrueand don't enter any values for content/wide width, and the root/default values are used, and no container classname is added - Toggle
Inner blocks respect content widthtotrueand enter values for contentSize, and a container class is added with the desired max-width on the children:
Overall this seems like a really nice way to remove the confusing idea of inheriting from the default layout, kudos for digging into how to get us from the current state of things to a more ideal representation of this kind of state! 👍
For the questions:
Do we also want to change the layout type for Post Content and Column blocks? That would require using the same logic in their Edit functions as we're applying to Group here.
If we don't make changes to these blocks in this PR, will this PR introduce regressions for existing blocks out in the wild? Or to put it differently, is this PR a breaking change for all blocks that have opted in to the default layout type? 🤔
We might need to put together some test markup to take a look at the impact of the changes on those blocks.
Is "column" really the best name for this layout type? Suggestions most welcome!
I can't quite think of a good alternative, but I think it'd be great if we can think of something that helps distinguish it from the Column block as I imagine it could be confusing the two using the same name. I suppose one question for the naming of the type is: are there additional features we'd like this layout type to have, that could help clarify its intended usage? I can't quite think of anything that isn't super clunky, like "flow with content sizes" or "constrain child content sizes"... ah naming is hard 😅
This PR changes a fair bit, so it might take me a little while to review in detail. I'll add comments here over the next couple of days, with things I find during testing, if you don't mind! I've added a comment about the post title wrapper that I noticed immediately.
| @@ -15,6 +15,12 @@ | |||
| "templateLock": { | |||
| "type": [ "string", "boolean" ], | |||
| "enum": [ "all", "insert", false ] | |||
| }, | |||
| "layout": { | |||
There was a problem hiding this comment.
Why is this being set here since this attribute comes from the block support?
There was a problem hiding this comment.
If we set the default in the block support, it would break back compat for existing blocks. Adding it as default here, together with the effect that's causing problems (#43760) in the edit function, ensures new blocks get the correct layout while existing blocks aren't changed.
If you have any ideas on how to do this better, I'm happy to explore them!
This seems like the main reason for this new layout right? and if I'm understanding right, this is the same thing as applying "inherit" by default. So why did we decide to introduce a new layout instead of just making "inherit" true by default? |
Mainly to make it easier to set it as the default layout type on new blocks, but it also had the nice side effect of allowing us to make the content width styles base styles (with content and wide width output as custom properties at root level), so we don't have to output |
I might differ in opinion because I feel like constrained and flow are just the same thing with different configs and we're tweaking the concept of "layout" for convenience rather than actual semantics but anyway, seems a bit late to alter it even more.
I think it's just not possible, |
I was afraid you might say that 😅
That sounds like an acceptable compromise. Could we name the variation "Group" so that the creation flow doesn't change? |
Yes, I think so. |
andrewserong
added
the
[Feature] Layout
What?
Fixes #33374.
New layout type
The idea is that, when creating a new block type, we should only have to add
to its
block.jsonand the block will apply content width to its children by default.wp-containerstyles for them (this is necessary so the default state of the column layout can access those styles).Changes to Group block and post editor layout
Changes the Group block so newly-added blocks are automatically set to "constrained" layout. Note: for back-compat reasons, we couldn't just change the default layout type, so we're adding some logic to the block Edit to set the layout attribute for all new blocks.
Adds deprecation/migration so older group blocks with content width can move to using the constrained layout.
The post editor appearance and alignment settings for top-level blocks used to depend on the "default" layout config; this is now changed to use "constrained" layout as the full/wide width options are no longer available for "default".
Changes to the sidebar Layout controls
The layout "full width" toggle will now switch from this new layout type to the default "flow" layout.
Changes the wording on the toggle to "Inner blocks respect content width" so that the default (flow) layout state is off.
The custom content width options now appear when content width is enabled via the toggle.
Things to consider
* Is "column" really the best name for this layout type? Suggestions most welcome!Why?
#33374 has shown that for most container blocks in most cases, not having a content width by default is unexpected and puts the burden on the theme author to manually change the setting every time.
This changes the most used container block (Group) to have content width by default, and provides an easy way of enabling content width by default for new blocks.
Testing Instructions
Using a block theme:
Using a classic theme:
theme.jsonto the classic theme.Screenshots or screencast