Add missing controls to flex layouts. #47134
Conversation
andrewserong
added
[Feature] Layout
|
Size Change: +253 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in ae4cc22. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4021416448
|
This one works well and seems to be an implementation of
This one works less well, because the settings only have any effect when the containing Stack is taller that the sum of its parts. Since it's not possible to define a custom height on a Stack, that's a rare occurrence unless I'm missing something? Most of the time you'll see this, which feels kind of broken:
Edit: I suppose if a Stack is placed inside a Row which has stretch-to-fill applied, then this setting would then have an effect. So I guess the question is whether we should attempt to detect that condition before displaying the control, or not.
This one works well too, with the except of blocks that have a specific width applied, like Images. For example this seems a bit strange:
The block is clearly stretching as intended, but the image retains its width. I'm not entirely sure what to do here, but I think my expectation would be that The icons need some thought, and potentially a separate discussion. |
|
Discussion for icons here: #47153 |
Outside of redesigning all the icons (which can still happen, but could potentially happen on a separate track), am I correct that the primary icons missing are a new vertical space-between, and a new vertical stretch to fill? For vertical space between can we just rotate the horizontal space-between, and then also make a version of that which stretches the square inside to fill for the other? Might not be perfect, but might unblock this one, buying time to explore the new icons better? (I.e. it might be a few mins. of work to make those two.) |
|
If we're happy with the |
|
From the discussion in #47153, here are the svgs: Space-between horizontal Space-between vertical Stretch horizontal Stretch vertical |
@jameskoster you can set min-height on Stacks now. |
tellthemachines
force-pushed
the
add/flex-controls
branch
from
da351bf
to
ae4cc22
Compare
|
Just noticed that it looks like the PHP unit tests for Also, in case an earlier comment got lost in the backscroll, what do you think about updating the value for space between so that it's kebab-case when serialized to the block markup to match the existing |
| @@ -49,7 +59,7 @@ function BlockVerticalAlignmentUI( { | |||
| const UIComponent = isToolbar ? ToolbarGroup : ToolbarDropdownMenu; | |||
| const extraProps = isToolbar | |||
| ? { isCollapsed } | |||
| : { popoverProps: { POPOVER_PROPS } }; | |||
| : { popoverProps: { variant: 'toolbar' } }; | |||
There was a problem hiding this comment.
This change fixes the following React warning in the console:
Warning: React does not recognize the `POPOVER_PROPS` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `popover_props` instead. If you accidentally passed it from a parent component, remove it from the DOM element.
|
Thanks for the feedback folks! And for providing the new icons too 😄 I think I've addressed all the remaining feedback.
Yeah, this is a tough one because the width is set inline on the image block, and also because we may not want to auto-enlarge an image with a small intrinsic size, as it'll end up looking fuzzy or pixelated, which might not be intended 😅 There's a similar issue with the Button blocks, because they have their own width settings. We should probably look into what the desired behaviour is there, too! But I think these would best be addressed as follow-ups as they'll likely be a bit of back and forth until we reach the optimal solution. |
|
@tellthemachines are we aiming for 6.2 with PR? |
|
Not sure if this is related to this PR or not, but while testing (which btw it was working very nicely for me), I saw that the column block doesn't have the alignment controls on the settings tab, just the toolbar:
|
There was a problem hiding this comment.
I'm approving from a functionality perspective. It's working really well in my testing. Great work 🙌
This is likely a separate PR, but it would be nice to have the vertical alignment in the settings tab as well. Much like @MaggieCabrera mentions above regarding the Columns block.
|
A big +1 for this PR, as I am working on a design that needs this functionality. I hope it makes it into 15.1 and 6.2. |
Yes!
The alignment controls don't appear in the sidebar for any blocks yet. That's definitely something for another PR, as it'll need some design guidance 😄 Thanks for reviewing and testing, folks! |
There was a problem hiding this comment.
This is mostly working really well for me @tellthemachines! I found a few little issues:
When toggling between horizontal and vertical, with a content justification value not present in the other orientation, the icon looks partially selected even though it kind of isn't. What should happen when we switch orientations here?
I noticed with the Buttons block, that the stretch item doesn't show up in the toolbar justification dropdown, so the selected icon appears to be the justified icon (the icon in blue in the toolbar):
Another issue with the Button block's styling is that it doesn't quite appear to handle the stretch option, since the Button is styled on the child a tag. So, the button block itself .wp-block-button is stretched to be full-width, however its child isn't.
Would it be worth adding a rule to the Button block's CSS to handle the stretch case? I imagine it could be a similar rule to the existing rule for .has-custom-width (here), where we'd set .wp-block-button__link to width:100% if .wp-block-buttons.is-vertical.is-content-justification-stretch? Though there could be a simpler rule for it 🤔
These probably aren't blockers to merging though, and could be looked at in follow-ups, I think.
| @@ -293,6 +313,13 @@ function FlexLayoutJustifyContentControl( { | |||
| icon: justifySpaceBetween, | |||
| label: __( 'Space between items' ), | |||
| } ); | |||
| } else { | |||
| // Todo: we also need an icon here. | |||
There was a problem hiding this comment.
Can this comment be removed now?
I'm not sure! Maybe it would be best to switch to default (flex-start). I'll do a quick follow-up to address the remaining issues. |
That sounds good to me 👍 Thanks for following up! |
Adds missing controls to flex layouts in `wp_get_layout_style()` for justify-content and vertical alignment options. References: * [WordPress/gutenberg#47134 Gutenberg PR 47134]. Follow-up to [54274]. Props isabel_brison, andrewserong, jameskoster, joen, onemaggie , ndiego. Fixes #57602. git-svn-id: https://develop.svn.wordpress.org/trunk@55201 602fd350-edb4-49c9-b593-d223f7449a82
Adds missing controls to flex layouts in `wp_get_layout_style()` for justify-content and vertical alignment options. References: * [WordPress/gutenberg#47134 Gutenberg PR 47134]. Follow-up to [54274]. Props isabel_brison, andrewserong, jameskoster, joen, onemaggie , ndiego. Fixes #57602. Built from https://develop.svn.wordpress.org/trunk@55201 git-svn-id: http://core.svn.wordpress.org/trunk@54734 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Adds missing controls to flex layouts in `wp_get_layout_style()` for justify-content and vertical alignment options. References: * [WordPress/gutenberg#47134 Gutenberg PR 47134]. Follow-up to [54274]. Props isabel_brison, andrewserong, jameskoster, joen, onemaggie , ndiego. Fixes #57602. Built from https://develop.svn.wordpress.org/trunk@55201 git-svn-id: https://core.svn.wordpress.org/trunk@54734 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Adds missing controls to flex layouts in `wp_get_layout_style()` for justify-content and vertical alignment options. References: * [WordPress/gutenberg#47134 Gutenberg PR 47134]. Follow-up to [54274]. Props isabel_brison, andrewserong, jameskoster, joen, onemaggie , ndiego. Fixes #57602. Built from https://develop.svn.wordpress.org/trunk@55201 git-svn-id: http://core.svn.wordpress.org/trunk@54734 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Why?
Fixes #45868 and #46133 as well as making #45117 possible. Complementary to #47099 - technically this PR provides the controls to solve all the layout problems in those issues, which #47099 addresses the problem of which is the most sensible default aligmnent value for flex containers.
How?
justify-contentfor Stack):Todo:
We need some icons for "stretch" and also for "space-between" in a vertical context, cc @WordPress/gutenberg-design
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast