Add Layout controls to children of Flex layout blocks #45364
Conversation
|
|
tellthemachines
added
the
[Feature] Layout
|
Size Change: +15.6 kB (+1%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
In case it helps for looking into width controls: there was an earlier working exploration into adding a width support back in #32502 from @aaronrobertshaw, and I've got a current PR that's looking into adding |
tellthemachines
force-pushed
the
try/row-width
branch
from
c363f06 to
a0c25aa
Compare
tellthemachines
requested review from
spacedmonkey and
ellatrix
as code owners
tellthemachines
requested review from
ramonjd,
andrewserong and
aaronrobertshaw
There was a problem hiding this comment.
Great work so far @tellthemachines, this is testing pretty well! Added a few questions and thoughts.
We probably don't want "Child layout" to be the name of this section; I called it that for now to tell it apart from the "Layout" section in blocks that have both:
This might require some refactoring (which we probably already need to do as part of #44560), but I was wondering if we can include these controls under the existing Layout panel? Because these controls only apply to children of a layout, I imagine we won't run into any collisions, so that a child of a layout can have these controls and their own layout controls in the same panel, which might be easier for a user to interact with. From the user's perspective (hopefully!) each of the controls there are things that affect the layout of that selected block, whether that's because of inherited / parent values or not?
Also, I like the choice to refer to this control as size, as opposed to Width, since the behaviour using flex-basis is quite different to the width CSS property. And separately to this PR there could still be good value in exploring a dedicated Width block support. This is likely a question for designers, but I wonder how this new control in layout would work alongside width controls (e.g. those currently in Button, Search, or Image blocks) 🤔.
A couple of other first impressions:
- Should this be an explicit opt-in / setting for layout? It's nice having this available for children of the Row block, but it seemed a bit unexpected for social icons to me.
- When setting a fixed size (e.g. 50%) the flex-basis value does not include a calculation using the current block spacing, so if I set 50% width on two children of the Row block, then the overall area will overflow:
Is it worth exploring adding in a calculation in the style output similar to how the width styling works on individual button blocks? The Button block's width control currently attempts to use the --wp--style--block-gap CSS variable, so falls apart if the parent Buttons block sets its own blockGap value — however, since this PR adds behaviour to the layout support, we should hopefully be able to grab the real value we want to use, if we incorporate similar logic here. Here's the Button block's rule:
Related to the issue of calculating width, when a fixed value is in use, should we also output a box-sizing: border-box rule in addition to flex-basis? Without it, if a Paragraph block has padding, then we also have the issue of the block overflowing unexpectedly:
Other than dealing with those finicky details, I think this is going to be such a useful feature, particularly being able to choose between Hug and Fill quickly without having to think about custom / specific CSS values.
| ...style, | ||
| layout: { | ||
| ...layout, | ||
| flexSize: value, |
There was a problem hiding this comment.
Is flexSize always going to be output as flex-basis? If so, I was wondering if we can use the latter, since the style object tries to stick closer to CSS properties where possible (where the root layout object appears to currently be a bit more abstract).
There was a problem hiding this comment.
I was trying to avoid giving it the property name in favour of a name that's clearer as to what it does, mainly because flexSize translates into more than one property (right now it's flex-shrink and flex-basis, but we might add border-box as per your suggestion, things might change etc.
There was a problem hiding this comment.
I was trying to avoid giving it the property name in favour of a name that's clearer as to what it does, mainly because flexSize translates into more than one property
Good point! Yes, for these ones that result in multiple / combined rules being output (or additional logic) that makes good sense to me, a bit like how blockGap doesn't directly correspond to gap 👍
I've also been wondering when it makes most sense to store things in the layout object directly or when to store them in style.layout. I quite like the decision to use style.layout here, personally, but I wondered if you had an idea in mind of when we make the call between the two? One possibility is when we aren't selecting from a controlled list of options (so things that aren't alignment based like content justification), style.layout might be the better fit?
There was a problem hiding this comment.
My idea here was to keep the layout attribute exclusively for blocks that have layout themselves, and use style.layout for styles that are related to a parent's layout.
| /> | ||
| <ToggleGroupControlOption | ||
| key={ 'fixed' } | ||
| value={ 'fixed' } |
There was a problem hiding this comment.
Could "fixed" cause confusion with the fixed/sticky position support once we have both of these features in? I wonder if there's another name we can use for entering a custom value 🤔
There was a problem hiding this comment.
That's a good question! I'd defer to design on that one 😅
| $child_layout_styles[] = array( | ||
| 'selector' => ".$container_content_class", | ||
| 'declarations' => array( | ||
| 'flex-shrink' => '0', |
There was a problem hiding this comment.
Oh, this is an interesting one. I assume this is to attempt to preserve what a user has chosen?
Will we always want to prevent the element from shrinking? That is, if someone sets a large value for flex-basis (e.g. 900px) I was wondering if we'll want to preserve that in smaller viewports where it'll cause an overflow, or in desktop viewports where it'll also overflow contentSize?
There was a problem hiding this comment.
Yes, if we set a fixed size we do want to prevent it from shrinking. That's touched upon in this comment, but also if we don't set flex-shrink there's no way to make sure the block actually sticks to the defined size 😅
As users are deliberately setting values, I think it's fine for them to overflow their containers where the values are too large.
There was a problem hiding this comment.
As users are deliberately setting values, I think it's fine for them to overflow their containers where the values are too large.
Ah, I see your point! It's a tricky one to work out the desired behaviour here (whether to honour what the user is attempting to do, or try to make sure they don't break the layout). I think I probably lean more toward preventing them from overflowing the container, because otherwise it breaks outside of the content size / wide size logic, which could be unexpected, and any width that's set greater than 400px would overflow most phone's viewports in a vertical orientation. Here's a paragraph set to 500px on desktop and mobile:
| Desktop | Mobile |
|---|---|
 |
 |
From a quick look at the Column block's ad hoc width control, it appears that it currently doesn't include a flex-shrink: 0 rule, so it currently ensures that Columns blocks don't break out.
Either way, though, since these CSS rules aren't baked into post content, it looks like it'll be a simple rule to change if we want to 🙂
There was a problem hiding this comment.
Interesting balance to strike on this one. In general, it seems like we try to start from a position of encouraging good design choices by default, then move towards honouring user choices. Following that, I'd lean a little more toward not allowing the broken layout first, then honouring explicit choices second.
That's only my naive two cents though, so take it with a grain of salt 🤷♂️
There was a problem hiding this comment.
I'm not sure it's helpful to think of this situation as a "broken layout", because people might want to create a horizontally scrolling page, and currently there's no way to do so in the site editor. It's easy enough to undo a change, and obvious enough that a horizontal scroll is happening if the user does add a bunch of fixed widths by accident, that we should let it happen.
Also, as I said before, there's no way of making this feature work properly without the flex-shrink in place. If we show a control to fix the width of a block, and that control doesn't actually fix it, that's a much more broken experience than allowing users the possibility of making their page scroll horizontally.
There was a problem hiding this comment.
I think it's a really interesting argument in both directions here (whether to honour the user's explicit choice, or attempt to do a fuzzy approximation). With other width controls (e.g. Columns or Search block), it looks like we currently don't allow overflowing the container? The search block sets the width value explicitly, but also has max-width: 100% so it doesn't overflow.
In terms of which user intent to prioritise, I'd be curious to hear thoughts from @jasmussen on this one — for a bit of history, there's been prior attempts to land a width block support, and the responsive behaviour was flagged on an earlier attempt here: #32502 (comment), and there's a subsequent open issue for responsive / intrinsic web design and how it relates to block controls in: #34641.
Whichever way we prioritise, I think it'd be good to figure out how we can support the one we haven't prioritised. E.g. if we went with the fuzzy approach, how do we then allow folks to intentionally create horizontally scrolling layouts, and alternately, if we honour the explicit pixel width, how do we allow folks to set a larger size that then shrinks in smaller viewports.
There was a problem hiding this comment.
It was @jasmussen's recommendation here to let explicit widths break out of their container 😄
There was a problem hiding this comment.
Ah, good catch, thanks @tellthemachines, I missed that one! That's a more recent comment so more likely captures the current thinking 🙂
There was a problem hiding this comment.
No worries, it's always good to poke at stuff anyway and see how we can improve it!
I think in this case the fuzzy approach would add too much complexity for the value it might bring: removing the flex-shrink would stop the setting from working as expected, so we'd probably have to do some calculation of parent widths or something horrible like that 😬
Also, given we already have Columns with the width constraints, it makes sense to have this behave differently, so folks can use Columns for a content-width-respecting layout and Row for more freestyle options.
I'm actually pretty excited to finally have a way to create fixed width, horizontally scrolling pages tbh 😁
There was a problem hiding this comment.
Great points, and there's nothing stopping us from adding a fluid or fuzzy option as an additional control alongside fixed in a follow-up at some point to handle the case where someone wants to set a target width that gets either shrunk or clampified 👍
|
Took it for a spin. Here's the row block before, with no width controls:
Here it is after, with new controls:
First instinct: this is going to afford some excellent new designs and adds incredible power to the flex blocks. Well done — fill, hug, and fixed width controls seem to be working as well as I hoped they would. Nice. A few thoughts on the UI, though. We should seek to avoid a new panel if at all possible. The designs in #44467 put it inside the Layout panel, which at that point is a toolspanel to allow defaults.
The tools panel as well as defaults (for example showing some controls by default if a parent, showing others if a child) is understandable to be a challenge, but it still seems best to keep all controls inside the same panel for now. Possible? Should we add the contextual help text from the mockups? There should ideally be 8px gap between the segmented control and the "fixed" width input field, a la this grid:
That may be on the component level, just as the 40px control height is, not sure, just want to make sure it's noted and that this will benefit from any component-wide changes made. Can we call it "Width" instead of "Relative size"? I can imagine that the size moniker was chosen because the parent can be switched to be a stack instead. But can we change the label to "Height" on the stack instead? What do you think? Nice work. |
|
+1 to everything Joen said, including "nice work", this is really going to unlock so many ways to express layout. One thing I noticed: if you create a Row containing two paragraphs, both set to width.mp4Code example to replicate: |
tellthemachines
force-pushed
the
try/row-width
branch
from
9e49e4c to
e3532db
Compare
|
Thanks for the feedback and testing everyone! We can make these child controls part of the main layout panel, no problem. In case a given block has both parent and child controls, how should we organise them? (I guess once we move to using tools panel, the question will be which should be visible by default?)
Yes, it should be! Funnily enough, I looked at Buttons and Navigation, and not seeing the controls appear, assumed they weren't working for any other blocks (not sure why - I'd expect them to just work wherever we're using flex layout) but forgot to check Social 😅 I do kind of like the aesthetic possibilities:
but notice the values set aren't being saved, so best turn this off for all blocks except Group until we work out what to do.
So this is where I don't think we'll be able to hack it with just the child controls. I left a note on the issue suggesting we have a parent-level control for fixing all children at the same width. That way we can make it Just Work independently of how many children exist, or are added/removed later.
⭐ excellent idea!
We should be able to do this, I'll look into it! |
|
Thanks for the follow-up!
Actually, yeah that example there is super cute. I suspect that once these controls are rolled out more broadly there'll be heaps of opportunities for fun and creative layouts. Very exciting! 😀 |
|
I'm a bit late to this party but to play devil's advocate quickly, am I the only one that took a double take at "width" or "size" controls not being under the dimensions panel? I do appreciate their ties to the layout support though. As a user with no background in CSS, our layouts etc., where would they expect it? This might be more of an issue if those controls aren't displayed by default and therefore hidden within a ToolsPanel ellipsis menu. |
Good take! I have a sense that many of the designs are inspired heavily by Figma where these properties are intrinsically a layout property, defining items and containers lay out their contents. But I could see it live in Dimensions as well. @jameskoster any hot takes? |
|
Good catch, the Dimensions panel would be a better fit. Layout is likely best reserved for container blocks – a rule of thumb for me to keep in mind.
It may not be a big deal – my expectations are likely biased based on how Figma behaves :) Looking back at the codepen the behavior is actually working as expected. All that to say: no need to add a parent control as a part of this PR, and may be not at all. |
This is an interesting point. My personal take is that it would be better for all the controls currently under Dimensions to be under Layout instead, because the only one we have in there that actually changes dimensions is the recently added Min height 😅 The advantage of having these controls together with the Dimensions ones is that when we display the flex-child width/height control we can easily hide the min-height one, and any potential width control we may yet add, because they'd be redundant. I guess the disadvantage is that it might be confusing to have width/height appear as options only when the block is within a Row/Stack block, and to have width switch to height if the parent changes from Row to Stack, for instance. Maybe we could mitigate this with some helper text explaining that this control depends on the parent's layout? |
|
That surfaces a good point. If the new width control sits in dimensions, it might make more sense for a nested group, which has both child-facing layout properties, and parent-facing width behaviors. |
|
Not something to do here but it would be good to explore how we might eliminate the conceptual overlaps that exist between these two panels. But for now, since everything in the Layout panel currently relates to child blocks within a container, we should probably stick to that heuristic and move these width settings to the Dimensions panel. |
|
Testing Before:
After:
One of the first things I notice is that the text above the Toggle does not change when going between Hug - Fill - Fixed. I expected this to change. Dimensions / Layout seems to cross into each others territory. Should these be merged? |
bph
added
the
Needs Dev Note
|
Dev note (it's a short one so probably should be combined with other relevant layout/design tools) cc @bph @andrewserong Sizing controls for flex layout childrenA new Layout feature was added that allows the children of container blocks with Flex type layout to provide controls to change their relative size within their parent block. This feature can be added to the container block in its The controls for the child blocks will then be displayed under the "Dimensions" panel in the block sidebar. If the parent's orientation is horizontal the control will appear as "Width", and if it's vertical it will be "Height". Three options are available in this control:
|
Is it intentional that this is missing from the group block ? |
Absolutely! These controls are specifically for children of flex layouts. Group has a flow/constrained layout. |
|
I didn't realize before that this is still experimental. __experimentalLayout - should we actually publish about it. I know it will bring a few comments again from core committers. There is the idea that experimental APIs are not documented. This has been a fluid rule, though... |
…lasses. In #45364 (WordPress/wordpress-develop#3976) the Block Supports was extended to add layout class names using the HTML API, new in WordPress 6.2. The initial patch opened up two opportunities to refine the code, however: - There are multiple instances of the `WP_HTML_Tag_Processor` created when a single one suffices. (There is an exception in that a second processor is necessary in order to find an inner block wrapper). - The code relies on the incidental fact that searching by a whitespace-separated list of class names works if the class names in the target tag are in the same order. In this patch the use of the HTML API is refactored to address these opportunities and clean up a few places where there could be stronger consistency with other use patterns of the HTML API: - Multiple instances of the Tag Processor have been combined to remove overhead, extra variables, and code confusion. The new flow is more linear throughout the function instead of branching. - Updated HTML is returned via `get_updated_html()` instead of casting to a string. - The matching logic to find the inner block wrapper has been commented and the condition uses the null-coalescing operator now that WordPress requires PHP 7.0+. - When attempting to find the inner block wrapper at the end, a custom comparison is made against the `class` attribute instead of relying on `next_tag()` to find a tag with the given set of class names. The last refactor is important as a preliminary step to WordPress/wordpress-develop#5096 where `has_class()` and `class_list()` methods are being introduced to the Tag Processor. In that patch the implicit functionality of matching `'class_name' => 'more than one class'` is removed since that's not a single class name, but many.
…lasses. In #45364 (WordPress/wordpress-develop#3976) the Block Supports was extended to add layout class names using the HTML API, new in WordPress 6.2. The initial patch opened up two opportunities to refine the code, however: - There are multiple instances of the `WP_HTML_Tag_Processor` created when a single one suffices. (There is an exception in that a second processor is necessary in order to find an inner block wrapper). - The code relies on the incidental fact that searching by a whitespace-separated list of class names works if the class names in the target tag are in the same order. In this patch the use of the HTML API is refactored to address these opportunities and clean up a few places where there could be stronger consistency with other use patterns of the HTML API: - Multiple instances of the Tag Processor have been combined to remove overhead, extra variables, and code confusion. The new flow is more linear throughout the function instead of branching. - Updated HTML is returned via `get_updated_html()` instead of casting to a string. - The matching logic to find the inner block wrapper has been commented and the condition uses the null-coalescing operator now that WordPress requires PHP 7.0+. - When attempting to find the inner block wrapper at the end, a custom comparison is made against the `class` attribute instead of relying on `next_tag()` to find a tag with the given set of class names. The last refactor is important as a preliminary step to WordPress/wordpress-develop#5096 where `has_class()` and `class_list()` methods are being introduced to the Tag Processor. In that patch the implicit functionality of matching `'class_name' => 'more than one class'` is removed since that's not a single class name, but many.
…lasses. In #45364 (WordPress/wordpress-develop#3976) the Block Supports was extended to add layout class names using the HTML API, new in WordPress 6.2. The initial patch opened up two opportunities to refine the code, however: - There are multiple instances of the `WP_HTML_Tag_Processor` created when a single one suffices. (There is an exception in that a second processor is necessary in order to find an inner block wrapper). - The code relies on the incidental fact that searching by a whitespace-separated list of class names works if the class names in the target tag are in the same order. In this patch the use of the HTML API is refactored to address these opportunities and clean up a few places where there could be stronger consistency with other use patterns of the HTML API: - Multiple instances of the Tag Processor have been combined to remove overhead, extra variables, and code confusion. The new flow is more linear throughout the function instead of branching. - Updated HTML is returned via `get_updated_html()` instead of casting to a string. - The matching logic to find the inner block wrapper has been commented and the condition uses the null-coalescing operator now that WordPress requires PHP 7.0+. - When attempting to find the inner block wrapper at the end, a custom comparison is made against the `class` attribute instead of relying on `next_tag()` to find a tag with the given set of class names. The last refactor is important as a preliminary step to WordPress/wordpress-develop#5096 where `has_class()` and `class_list()` methods are being introduced to the Tag Processor. In that patch the implicit functionality of matching `'class_name' => 'more than one class'` is removed since that's not a single class name, but many.
Why?
Closes #44467
Follow up tasks (not within the scop of this PR):
safecss_filter_attrto allowbox-sizingrules to be output.How?
allowSizingOnChildrenlayout attribute to enable width/height controls on children of flex blocks.__unstableParentLayoutprop to BlockListBlock, which is used to decide whether those child controls should be output or not, based on the existence ofallowSizingOnChildrenand a flex type layout.Testing Instructions
Screenshots or screencast