Skip to content
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

Spacer: Add toggle to behave as flexible spacer to take up available space (flex-grow) #38022

Closed
jasmussen opened this issue Jan 17, 2022 · 15 comments · Fixed by #49362
Closed
Assignees
Labels
[Block] Spacer Affects the Spacer Block [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Status] In Progress Tracking issues with work in progress

Comments

@jasmussen
Copy link
Contributor

The Row block through its flex behaviors has introduced a number of possible layouts. However in many cases, these layouts require multiple nested containers. The following Navigation examples would 3 or 4 nested containers:
grouped left
centered chunk

By leveraging the power of flexbox we can theoretically eliminate those, specifically by using space-between, which lets items inside take up space according to grow and shrink properties. Right now, however, items inside don't grow or shrink, and just space themselves out, like so:

space between

Instead of enabling grow/shrink properties on individual items, we could allow a spacer block to grow, and simply take up available space:
Flexible Spacer

The toggle would exist only inside flex containers (Row, Navigation, others), and could potentially enable a range of layouts with fewer nesting containers and a simpler flow.

@jasmussen jasmussen added Needs Design Feedback Needs general design feedback. [Block] Spacer Affects the Spacer Block [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi labels Jan 17, 2022
@karmatosed
Copy link
Member

I really like this idea and can see it being really useful.

@jasmussen
Copy link
Contributor Author

@tellthemachines I wonder: since the navigation block is a flex container, could we enable the new width controls on all blocks inside? They would default to fit, but you could set them to fill. That would obviate the need for a separate flex toggle.

@tellthemachines
Copy link
Contributor

@jasmussen we already have them since #47584! I guess we can close this issue now?

@jasmussen
Copy link
Contributor Author

Oh nice! I'm not seeing it on the spacer block, though?

Screenshot 2023-03-20 at 09 38 06

@tellthemachines
Copy link
Contributor

The control is under Dimensions in the styles tab:

width-in-spacer.mov

In playing around with it I'm noticing that a fixed width in the flex child control overrides the actual spacer width, which makes it feel very broken. Not sure what the best solution here would be, but given that Spacer was introduced to Nav in order to do pretty much what those child controls are doing, should we consider removing it?

The only downside I can see to removal (aside from back compat; we'd have to allow existing Spacers to remain in place of course) is that with the flex controls, in order to add extra space between two items, the parent block has to have some extra space itself, whereas adding a Spacer pushes out the parent block and creates whatever space is needed.

@jasmussen
Copy link
Contributor Author

Ah, nice, thank you for the video, that's helpful. It seems like we definitely want only one width control, not two. Can we remove the control in the interface? Any widths already set using that control would still be saved, but going forward things would be unified. There's a question about the resize handle — could that be made to apply the new flex width, and set it to "fixed"? Probably not needed for a v1 of this, mainly thinking in terms of future enhancements.

the parent block has to have some extra space itself, whereas adding a Spacer pushes out the parent block and creates whatever space is needed.

You mean that there has to be some available space, lest through its flex nature it collapses? Wouldn't setting a fixed width expand the spacer? In any case, fit/fill spacers in flex containers are somewhat advanced territory, and I don't think it's unreasonable if you're in that territory that you have to either use arrowk keys to page through blocks to select them, or use the list view.

@tellthemachines
Copy link
Contributor

Hmmm this is an interesting problem.

We do have access to the __unstableParentLayout property from the Spacer block's edit function, which would allow us to hide the Spacer's native "width" or "height" control when inside a flex container, but we also have to consider the resizable handles in the canvas. For the handles I see two possibilities: we could remove them altogether when inside a flex container, or we could try to adapt them to handle fixed flex child values instead of the native Spacer width and height properties.

This might create issues when moving a Spacer out from a flex container, especially if it happened to have a fixed size in percentage units, because the native Spacer control doesn't allow percentages.

Deprecation-wise we should easily be able to convert existing width/height values to flex-basis.

@tellthemachines
Copy link
Contributor

Another thought: the fact that it's so unintuitive to go looking for the width/height controls under "Styles" reinforces some of the arguments in #47902 about unifying sizing and layout tools under the same panel.

@jasmussen
Copy link
Contributor Author

which would allow us to hide the Spacer's native "width" or "height" control when inside a flex container, but we also have to consider the resizable handles in the canvas. For the handles I see two possibilities: we could remove them altogether when inside a flex container, or we could try to adapt them to handle fixed flex child values instead of the native Spacer width and height properties.

Seems fine to hide the handles inside a flex container, at least as a quick bugfix. We can always explore introducing resize handles across all flex children in a generic way later. As for how they should behave, I would imagine that any manual resize sets a px + "fixed" value.

I realize that we have several width height controls now, both those for flex and for regular width/height:

Screenshot 2023-03-22 at 08 55 07

This is very probably worth a separate issue (CC: @WordPress/gutenberg-design for a sanity check), but it would be nice to unify these. Whether in dimensions or otherwise, width is width, and it would be nice if a width/height control could adapt depending on whether the container was flex or not.

@jameskoster
Copy link
Contributor

Hmm, the fit width option doesn't really make sense for the Spacer, we should probably remove that option.

Seems fine to hide the handles inside a flex container, at least as a quick bugfix

I don't have a strong opinion, but I think it would be fine if the handles effectively functioned like a toggle from fill to fixed, and applied width using the Dimensions > Width control rather than the native Spacer width.

I agree at some point we should make that transition (remove the native controls in favor of Dimensions > Width / Dimensions > Height).

@richtabor
Copy link
Member

I realize that we have several width height controls now, both those for flex and for regular width/height:
This is very probably worth a separate issue (CC: https://github.com/orgs/WordPress/teams/gutenberg-design for a sanity check), but it would be nice to unify these. Whether in dimensions or otherwise, width is width, and it would be nice if a width/height control could adapt depending on whether the container was flex or not.

I ran into this as well, with the core/site-logo block (#47979).

@tellthemachines
Copy link
Contributor

The Site Logo issue, along with Image (see also #12168) is a much tougher problem to solve because the img element in both those blocks is wrapped in a bunch of layers and resizing the block's outer wrapper does nothing to change the dimensions of the image itself. The fit/fill/fixed controls adjust flex properties that have to be applied to the outer wrapper, so they don't really work for the image type blocks.

I think as a starting point it would be good to define exactly how we'd like images to behave within flex containers, as well as outside of them, considering that e.g. if we were to reset the intrinsic dimensions of an image so we could make it resize to "fill" a flex container, a low res image might end up not looking so good:

Screenshot 2023-03-23 at 5 13 10 pm

I'm also noticing that Spacer inside a Row doesn't get the proper orientation:

Screenshot 2023-03-23 at 4 54 26 pm

@tellthemachines
Copy link
Contributor

Another thought: unifying width/height controls should take into account the ability to set/unset values globally through theme.json, as described in #27614.

@richtabor
Copy link
Member

I wonder if we need the Settings panel "Height/Width" control anymore—perhaps this is an opportunity to leverage the Dimension > Width control instead.

We already have a Dimensions > Width control (although it requires the Row block as a parent block). If we can leverage that, it would further reduce the block to one panel as well—another win.

Could look something like this:

CleanShot 2023-03-28 at 16 46 25

I'm using the existing Dimensions > Width control UI, as well as the upcoming Spacer block presets PR in the mockup.

What we'd need to do:

  • Enable only "Fill" and "Fixed" width (we don't need "Fit").
  • Set "Fixed" as default (should we reorder the buttons?)
  • Allow for the Spacer block's Dimensions > Width control to render without existing within a Row block.
  • If within a Row/Stack block, then the Fill/Fixed control is available.
  • Pass a control for the Dimensions > Width control to use when "Fixed" is selected. That way it can be leveraged instead of the default input unit control (i.e. the spacing presets).
  • The additional fixed control feels odd without a label. Currently they both say "Width". That won't work, but kept it as so for illustrative purposes.
  • "Margin" should turned off as a default tool — which should probably happen regardless of if we go this path or not (no other blocks have it set as a default tool).

Maybe it's too much? Just thinking a bit outside the box to consolidate these controls and the new fill idea.

@jasmussen
Copy link
Contributor Author

I wonder if we need the Settings panel "Height/Width" control anymore—perhaps this is an opportunity to leverage the Dimension > Width control instead.

Agreed.

@richtabor richtabor removed the Needs Design Feedback Needs general design feedback. label Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Spacer Affects the Spacer Block [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants