Add Width and Height controls to children of flex, flow and constrained layouts #59195
Conversation
tellthemachines
added
[Type] Experimental
|
Size Change: +1.08 kB (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in da5d397. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8015701363
|
|
This pull request changed or added PHP files in previous commits, but none have been detected in the latest commit. Thank you! ❤️ |
tellthemachines
force-pushed
the
add/child-width-and-height
branch
from
8cfa66e
to
e482872
Compare
tellthemachines
requested review from
noisysocks,
SaxonF,
richtabor,
ramonjd and
andrewserong
|
Marking this ready for review to get feedback on the approach; will add tests once we're agreed on it. |
|
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. |
There was a problem hiding this comment.
I left mostly irrelevant comments, just while perusing the changes, and getting lost wondering if layout.php will ever benefit from a code refactor/abstraction.
Overall it's working as described, and I didn't run into any friction. Great work!
Here's what I'm seeing:
- Height and width controls appear for children of container blocks
- The controls don't appear for directly children of grid layouts
- Height and width values match expected behaviour, e.g., fit, fill and fixed
At first I was a asking myself what "Max width" meant, given that it output flex-basis: ${value} - had recheck the flex docs. So, just to confirm it'll shrink according to the container space and then be no wider than ${value}?
lib/block-supports/layout.php
Outdated
|
|
||
| // Orientation is only used for flex layouts so its default is horizontal. | ||
| $parent_orientation = isset( $block['parentLayout']['orientation'] ) ? $block['parentLayout']['orientation'] : 'horizontal'; | ||
| $vertical_parent_layout = in_array( $parent_layout_type, array( 'constrained', 'default' ), true ) || ( 'flex' === $parent_layout_type && 'vertical' === $parent_orientation ); |
There was a problem hiding this comment.
Given this is a bool, maybe something like $has_vertical_parent_layout?
lib/block-supports/layout.php
Outdated
| @@ -573,14 +573,72 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) { | |||
| $child_layout_styles = array(); | |||
|
|
|||
| $self_stretch = isset( $block['attrs']['style']['layout']['selfStretch'] ) ? $block['attrs']['style']['layout']['selfStretch'] : null; | |||
| $self_align = isset( $block['attrs']['style']['layout']['selfAlign'] ) ? $block['attrs']['style']['layout']['selfAlign'] : null; | |||
There was a problem hiding this comment.
Is $block['attrs']['style']['layout']['selfAlign'] the same as $child_layout['selfAlign']?
There was a problem hiding this comment.
Yes! I probably pasted this in from the prototype and forgot to make it pretty 😂
| const id = useInstanceId( useBlockPropsChildLayoutStyles ); | ||
| const selector = `.wp-container-content-${ id }`; | ||
|
|
||
| const isVerticalLayout = | ||
| parentLayout.type === 'constrained' || |
There was a problem hiding this comment.
Could this be parentLayoutType?
There was a problem hiding this comment.
Oh good catch, it should be!
| // The layout settings from the block's block.json. | ||
| ...defaultLayoutBlockSupport, | ||
| // Any alignment set on the block. | ||
| ...( align && { alignWidth: align } ), |
There was a problem hiding this comment.
I'm no guru in this area, but it makes sense to me to make inner blocks aware of their parents' alignment 👍🏻
There was a problem hiding this comment.
Nice progress! Just jotting down a few thoughts as I went through and tested some of it. Apologies if it's a little messy!
When I go to add a Cover block, it throws an error for me (supportedAlignements?.includes is not a function):
When I add a Paragraph with a background colour to a constrained Group block and give it a "Fixed" width, it can extend beyond the edge of the viewport / outside of the constrained area:
2024-02-23.14.48.16.mp4
When adding an Image block to a constrained Group, the height controls don't appear to work as expected. Because of the potential for rule / styling conflicts, I wonder if it'd be worth it to make the controls opt-in?
2024-02-23.14.52.12.mp4
The option to be able to set a Max Width sounds good to me, but how will Max Height work for overflowing content when reducing the width of the viewport? I.e. if a user lays content out in such a way where the max width looks good on desktop, what will happen in narrower viewports?
One other thought in the back of my mind: how might the width / height rules play with the new aspectRatio support? If width / height is updated, would we want to clear out any aspectRatio values, or vice versa?
Overall, though, this is a really exciting set of features! I imagine there might be a few older issues and requests that could be closed out once it's done, as folks have asked for width / height block support for a long time, and exposing the controls to children of the constrained/flow layouts seems like it'll get most if not the rest of the way there 👍
tellthemachines
force-pushed
the
add/child-width-and-height
branch
from
e482872
to
c2c5c4e
Compare
|
Thanks for reviewing and testing folks!
Probably! There's some repetition in the block gap logic.
Cover block strikes again 👀 I'll look into it!
Yes! That's what it's supposed to do. There were complaints about the "Fixed" option in the flex child controls not being really fixed, so this time fixed is 100% not trying to do anything smart behind the scenes. "Max width" (and "Max height") are basically the same as the previous fixed-not-really-fixed option; I guess we could add them to constrained/flow layouts too?
Yeah that's an interesting problem. It's an existing issue when adding fixed height to an Image block inside a Stack block with the flex child controls, and I'm tempted to address it separately with a fix in the actual Image block, somewhat like we did for Spacer in #49362. I don't think making the controls opt-in would be viable because we want them to ideally work for all blocks; it's preferable to try and unify controls where possible.
If I built this correctly 😅 there should never be an overflow for max-width because it always adjusts to the container width.
Ah yes, good point, we'll have to do something about that! I imagine something along the lines of the current relationship between aspectRatio and Min height, where setting one unsets the other? |
|
Ok Cover block issue fixed! |
tellthemachines
force-pushed
the
add/child-width-and-height
branch
from
da5d397
to
18170cf
Compare
Ok so I've added the "Max width" option to all vertical layouts. The slight issue here is that currently, setting it on a child of a constrained block gets overridden by the default layout max-width 😅 My attempts to reduce layout CSS specificity over in #57841 were not entirely successful; it's unlikely that the generic (I also realised in testing that Fixed widths aren't working properly in constrained layouts either so might need a It's not great, but I'm inclined towards |
Just thinking a bit more about this and we might be able to remove the This inclines me further towards a temporary addition of |
Ah, very interesting! This is quite likely my own personal preference, but to me it makes the controls harder to use if they don't include automatically handling things gracefully reflowing on smaller screens. My expectation when I was playing with it was that I'd be able to break out of the constrained layout (so, go wider than the column I'm in), but it'd still be reduced down to the viewport width, otherwise it felt like it'd be easy to break layouts on narrower viewports. I think most of the time folks won't want to accidentally create horizontally scrolling content. Max Width gets us pretty close to that, but while using the current state of the controls, I found myself wanting something that's somewhere between Max Width, Fill, and Fixed. In any case, I'm not a designer, so it'd be great to see what others think, and what covers the majority use case of folks reaching for these controls.
They get most of the way there, so sounds good to me!
Ah, I see, because it's the parent that sets
My comment wasn't worded very well, but I was actually thinking about the height overflowing. There's a couple of different ways it can happen at the moment, but here's a simple example when setting a Fixed height, where it looks good at a desktop size, but overflows in narrower viewports:
It's similarly possible to wind up with overflowing content with a child of a Row block when setting a max height: 
From memory, I think there were some similar conversations with the aspect ratio controls. The way that the aspect ratio wound up working in the end is that if the content is longer than the aspect ratio, then the size of the block grows with the content, so we could avoid the scrollbar or overflowing content problem. Not sure if that's really applicable here, though. I think @jasmussen or @jameskoster might have mentioned the idea of exploring controls for overflowing content one day 🤔
Unsetting an aspect ratio value when one of these dimensions values is set sounds good to me! One other thing I noticed while re-testing, is that if I enter a Width and Height, the ToolsPanel menu only sees a change to Width and not to Height: 
|
|
For me the main question is about user expectations regarding the behavior of elements with fixed heights. Would users anticipate overflowing, clipping, or scrolling? My inclination is to implement one of these behaviors now and allow customization through overflow controls later. |
tellthemachines
force-pushed
the
add/child-width-and-height
branch
from
b812688
to
d14e3eb
Compare
I would lean towards hidden by default as it seems the safer starting point. What do you think @jameskoster ? |
Oh bugger, I always forget about the tools panel resets 😬 |
Actually, I think we'll have to let ChildLayoutControl absorb the rendering of ToolsPanelItem because we'll need a ToolsPanelItem for each of the Width and Height controls. |
|
Ok ToolsPanel resets are now split (except for Grid column and row, but that's probably better done as a separate task if we feel it's needed) Remaining issues are:
I'm reluctant to add a new API until we know for sure it's necessary; to address the Image block issue we could follow up with another PR to unify these controls with the Image block ones. I don't think it's hugely problematic to leave the controls enabled for it in the meantime; we already have flex child controls being used on Image blocks (so disabling would sort of break back compat there too) and for the most part the flow/constrained addition will work well with existing align wide/full Image support. It's also worth mentioning that I'm not worried about custom blocks here, because most of them strip out the whole sidebar styles/settings and replace them with their own 😅 and the ones that don't usually work well with core block supports because they already use some or all of the supports. Fixed/max widths and heights are the only settings that don't always work properly, particularly when blocks already set width and height and/or have multiple wrappers. It could be worth looking at a skipSerialization option for these scenarios (not sure where it might best be added though - under dimensions?). From my testing Image is the flakiest one; then there are cases like Audio and Table where adding a fixed height just increases the space under the blocks. All of these could potentially be addressed case-by-case with adjustments so the controls work. We could also not add fixed/max widths and heights to flow and constrained children for now (we should maintain them for flex because they're partially supported already) but we'll be missing (once again) the opportunity to add functionality that has been requested over and over, which seems a shame. |
Agreed. I'm struggling to decide on scrolling though. For text you'd probably expect scrolling, but more decorative sections (e.g. image in a container) you wouldn't want scrolling. |
|
Hi There! Thanks for working on these two controls. I think these are great additions that will allow new sets of designs. These tools also offer new opportunities for consolidations and improvements for our existing APIs/tools. Note that some initial thoughts about this have been shared in #28356 A summary of some of the important points that I think we should consider:
|
|
Thanks for the feedback @youknowriad! Those are some interesting points, and I have a lot of thoughts about them, so please bear with me while I think out loud a bit 😅
I'm not sure! The only reason I can think of that dedicated width and height block supports would be useful is for specific blocks to opt out of them if necessary. #43241 is good evidence for questioning the initial assumption around block supports being opt-in: in the end, we do want most of them to be usable with most blocks, so it would be far easier to have a system that allows for exceptions than one that forces us to add everything explicitly to every single block. I guess the major point against is incompatibility with the existing implementation: width and height controls are already available for children of Row/Stack and Navigation blocks (width or height, depending on layout orientation), with the parent block opting in to the controls for all its children. This approach makes sense for the flex layout type, because blocks inside that layout need to be adjusted in relation to each other. The same applies to the column and row span controls for grid children added in #58539, which for practical purposes replace width and height controls for those blocks (width and height for grid children don't make much sense, because they are already constrained by the grid's column and row rules). Another thing to consider is that calculating the CSS for different width and height settings is also dependent on layout: making a block fixed or max-width inside a flex container is different to a flow container. Presets like content and wide width could kinda be made to work in a flex container (default flex shrinking behaviour complicates it a lot) but they would require different styles to do so.
We might have missed the boat on that one: flex layout was never put under appearanceTools so it could be available on all classic themes. When the flex child controls were implemented (as well as the grid layout later on) the same logic was followed.
That would be great! Separating "wide" and "full" from alignments won't be trivial though, due to back compat: even if we did decide that all core blocks should have all the alignments (which I'm totally in favour of 😄), we should still respect any filters applied to I think this should probably be a separate task from this PR. I don't think anything in this PR actually prevents that exploration? Assuming that we will have to preserve back compat for legacy align attributes, that is. |
For me width/height are different from layout. I can understand why layout would be made available automatically for all themes, it's not really a customization/appearance tools but I feel it's the case for height and width this is the reason why maybe a dedicated opt-in could make sense (that would be enabled by default with appearanceTools)
I agree it's separate and I agree back compat would be complex, I feel this is one of these things that we should first consider an experiment and I'm not sure a "migration" is possible to be honest, more like for new versions: hide these from alignments in favor of a maxWidth control. The relationship with this PR is that we might want to introduce width presets that work both for this PR and these alignments, which means I'm not sure we should support "wide", and "default" should be options to start with in this PR because they might create backward compatibility issues when we introduce "width presets". |
Width and height need to know about layout, though that wouldn't be a blocker to making them a separate support (just like block spacing). The blocker is that the width/height control for flex children is already available on classic themes that don't opt in to appearance tools, so changing that would break back compat. We could maybe make width/height for flow and constrained children an appearance tools opt in, but if it's already universally available for flex children I'm not sure there's much point to it 😅 |
I've updated the PR so that "default" doesn't show as a width option unless either the theme explicitly declares It should be fairly easy to add any custom width presets the theme defines in the future to this list so they appear as options in the dropdown. Whether we go with just allowing to add other options directly under the layout setting like or we define a new key like we'll probably need to keep "contentSize" as the default content size to be used in constrained layouts. Thinking that in the future we may want to use templates from any theme in any other theme, it will be helpful to have a universal content size variable for compatibility. In regard to classic themes, I've been testing these controls in a few of them, and they don't always work as expected. Classic themes usually define content widths themselves, and some of them (e.g. TwentyTwenty) do so with enough specificity to override the output from these controls. Because we won't be able to make the controls work consistently across classic themes, I'm inclining towards hiding them altogether (like we always hide the flow/constrained layout toggle, even if the theme does support |
|
Update: I've hidden the width and height controls in classic themes for children of flow/constrained layouts. |
While I can see that width might not apply as you expect in some layouts, I don't really understand why these are specific to layout. A width
That also doesn't sound great to be honest, I really don't think we should conflate "contentSize", "wideSize"... with widths presets. I feel we should consider these as legacy from the start and instead just treat width presets as any other presents: |
|
@youknowriad from a UI perspective though we have various controls that reference content/wide (eg align) but overlap with the idea of width. If I select a width value from presets that contentWidth uses, is that the same as setting align? |
No, the presets are just there to be reused in one or multiple places (like we do for colors for instances). So we could use them for "align" (which is basically maxWidth support) and we can use them for the new "width" introduced in this PR. |
We need different CSS to apply width in flex and block display types. So for instance, Even from a user perspective, adjusting width and height for a block will usually take into account the parent and the sibling blocks. It's making a block stretch to the full width of its container, or making two or three blocks in a Row have the same height, or two blocks side by side have the same width. I think semantically these controls naturally fit in with layout. |
Shouldn't we be adding flex-shrink automatically to all flex children? I can't think of cases where it's helpful to allow shrinking? Regardless, there's already a precent for block supports that have "small" dependencies to layout but are not entirely dependent on layout: fluid typography.
I'm not sure how prescriptive we should be, I've seen people ask for a possibility to do this (overflowing columns)
It's not entirely clear whether we should mix this one (flex-grow) with "width" or whether it should be its own checkbox. If it's a checkbox it's separate from width support (in fact for vertical flex, I believe its behavior is to grow the height) but even if we keep it within the width, as I said above, a small dependency to layout is fine. |
You'd think so! But after initially implementing "Fixed" widths for flex with no shrinking, some folks thought it wasn't a good idea, so I reverted it in #46139. Essentially, what we implemented there was a "Max width", not a "Fixed" width at all. I do think that there's a need for both, but the current controls are suboptimal because they do one thing (max width) and call it the other (fixed width). Really, this PR was intended more as a refinement of those controls. The extension of these capabilities to children of flow/constrained layouts makes sense within the context of #42385, as one of the goals is to be able to switch seamlessly between flow, constrained and vertical flex layouts (so they should all have similar controls). The other side of that is presenting all the layout-related tools in one section. It makes no sense to have block gap, width, height etc., in a separate "Dimensions" panel because usually these settings are adjusted in relation to each other.
I'd love to see some specific use-cases! I suspect if what folks want is to overlap columns, the column/row start controls experimentally added in #59483 would be the best way of doing that.
I'm going to trust @SaxonF on this one 😄 |
One of the benefits of treating this as a width setting is that its consistent with other design tools like Figma, which is important for making designers feel at home. I'd like to start here and pull out into its own thing if we get that feedback consistently. |
Would it be helpful to potentially further divide up this PR up into separate pieces? E.g. one PR that only looks at the redesigned controls for flex but keeps the styling output the same? (I.e. switches to the select dropdown, with Fixed renamed to Max Width) There's lots of fascinating ideas, questions and decisions with these controls, and I wondered if doing something like that might help sidestep the question of how flow/constrained width and height controls should work, while still progressing the goal of redesigning them. Or is it better for them to be considered together as a whole? |
This feels almost like the most important decision for me in addition to where we store the "width" value. Should it be The rest is mostly implementation detail. My opinion is that it should probably be: This keeps "width" consistent between layouts and avoid "special" values to be stored within "width". |
What?
Extracts the child controls from the layout redesign prototype in #53455.
UI changes
flow,constrainedorflexlayout types that opt in toallowSizingOnChildrenget a Width and a Height control in the Dimensions panel.Available widths
FixedandFitfor all blocksDefaultis specific to constrained layout children and applies content widthWideandFillare available for constrained layout children that support wide and full alignmentFillis available for all flow and flex children aswidth: 100%andflex-grow: 1respectivelyMax widthis available for horizontal flex children and is equivalent to thefixedoption on trunk (so not actually fixed but shrinkable)Available heights
FixandFixedfor all blocksFillandMax heightfor children of flex layouts, both horizontal and vertical.Implementation details
WideandFillalignment options only when the parent block is aligned wide or full, otherwise the controls seem like they're not doing anything. That differs from alignment behaviour on trunk, which has always felt weird to me so I was keen to try out something different.CustomSelectControlV1 but should be easy to switch once V2 becomes available.onChangeAlignmentfunction so that wide and full alignments can be manipulated from the Width and Height controls. We should be able to remove them once these controls move to the Layout panel because the block editor's DimensionsPanel is still a private API.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast