Adapt flex child controls for Spacer. #49362
Conversation
tellthemachines
added
[Block] Spacer
tellthemachines
requested review from
MaggieCabrera,
jasmussen,
andrewserong,
apeatling and
draganescu
|
Size Change: +221 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 7d751d2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4685437479
|
|
Nice, thank you for tackling this! Already seems like a nice iteration. One thing I'm realizing is that the navigation container itself might also need a width control. This GIF shows a navigation block that's only as wide as its contents inside, and so when I set a "fill" property to a spacer inside, it's zero wide because there's no space for it to fill out:
Presumably the "fill" property would work if the navigation container was 100% wide. What do you think? |
If that Nav block is inside a Row block, it'll have its own fit/fill/fixed controls under "Dimensions" (you might have to look for them in the dropdown because they don't show by default). If it's not inside Row then it should take up 100% of its container's width already. |
There was a problem hiding this comment.
Very cool idea, the child flex controls are going to be much nicer for playing around with spacing!
The syncing from dragging the handle on the spacer block to the Fixed width/height value in the flex controls appears to be working quite well. Unfortunately it looks like it isn't syncing the other way around when adjusting the value in either the horizontal or vertical orientations. I think it might be a matter of including the flexSize value in the getHeightForVerticalBlocks and getWidthForHorizontalBlocks functions?
2023-03-28.12.20.32.mp4
Other than that, now that the spacer controls are hidden for flex layouts, should we display the child flex controls by default so that the controls aren't hidden when a block is freshly inserted?
| if ( isFlexLayout && selfStretch === 'fit' ) { | ||
| return undefined; | ||
| } | ||
| return temporaryHeight || height || undefined; |
There was a problem hiding this comment.
Do we need to also use style.layout.flexSize in this value to sync changes to flexSize back to the styles applied to the Spacer?
| if ( isFlexLayout && selfStretch === 'fit' ) { | ||
| return undefined; | ||
| } | ||
| return temporaryWidth || width || undefined; |
There was a problem hiding this comment.
Same comment as above: do we need to also use style.layout.flexSize in this value to sync changes to flexSize back to the styles applied to the Spacer?
|
Thanks for reviewing! I've addressed the issue with resizing when the fixed value changes. I also noticed something else: the resizing handles always convert whichever unit is used in the input field back to
I'm starting to feel like they should always display for all blocks because I've had to explain multiple times where to find them, which probably means they aren't discoverable enough. |
Now that you mention it, I think that would make a lot of sense. Typically the controls that are hidden by default are ones that we think folks mightn't reach for all that frequently. When a block is a child of the Flex layout, there's a much higher likelihood that people are going to want to make adjustments to it. Might need some design feedback, and there could be some exceptions (Image block perhaps?) where there are other controls that might need to be more prominent than the flex children controls, but sounds like a good thing to explore to me! |
That seems okay to me. How would the resize handle be able to work with, say, percentages? (Apologies if I'm missing something obvious.)
Only on my second coffee so my mind isn't entirely clear. But at this juncture, I'm also leaning that way. However
That is to say, I still think there's a place for hiding advanced functionality by default and adding it, but mainly that it's not always clear what is advanced vs. what is basic. I also think @SaxonF may have had some thoughts around some of these dimensions migrating to the layout panel. I don't recall all the details, but just connecting the dots. |
|
This seems to be working as expected in terms of the changes here. 👍 Unrelated to this PR I noticed was that the tooltip shows "null" when resizing. I realize this is reflecting a height value that isn't set, but it felt like a bug and would be better to say something else? Is this related to the px conversion?
|
They might not work with percentages - that's probably why the custom Spacer unit control doesn't allow them - but we could conceivably make them work with rem, em, vw and vh. I agree it's not a super high priority though!
I'm not sure why this happens but given it's also happening on trunk I thought to investigate it separately. |
tellthemachines
force-pushed
the
try/spacer-flex-controls
branch
from
50c9ab6 to
c709721
Compare
|
I've solved the conflicts from the merge of #44002 - cc @glendaviesnz - and I think this is ready for another round of reviews! |
There was a problem hiding this comment.
This is generally working pretty well, but I noticed a couple of inconsistencies with how the flex child controls appear to work:
- In a Row or Stack block, when you insert a Spacer block, the flex child controls default to
Fit, but the spacer visually looks like it uses its default value (100px). Should it be defaulting to theFixedtab? - The same issue occurs if you set a Spacer block outside of the flex block, and then drag the block inside the flex block. The tab is on
Fitinstead ofFixed. Also, if you toggle between the options,Fitalways results in0size because there's no content.
Here's a quick screengrab of dragging a Spacer block into a Stack block:
Other than that, I also noticed that Fill in the vertical orientation sometimes behaves inconsistently for me if I don't have a min height value set. With min height set, it appears to be working quite well, though 👍
|
Thanks for testing @andrewserong !
Yes, I've fixed that now.
That's expected given Fit is based on content size. Ideally, we wouldn't display Fit at all in this context, but I can't think of any good way of doing that doesn't involve adding a block attribute to manage child layout controls. Does that sound like a terrible idea? 😅
Can you provide more details on the inconsistency? Fill shouldn't really work unless the parent has a min height, but I notice it seems to default to 100px. |
There was a problem hiding this comment.
Thanks for the update! Just left a comment about dragging a Spacer into the Stack block, it looks like the value might need to be updated there to also handle height.
For the Fill issue, I couldn't quite work out what's going on, but hopefully this video will help. It seems that the value set by dragging the control winds up overriding the Fill value possibly? Essentially, when I drag the spacer via the handle, then when I switch between Fixed and Fill, it'll remember the size based on the drag handle, rather than appearing to use Fill, if that makes sense? Here's a screengrab:
2023-04-12.16.52.23.mp4
Happy to give it another test tomorrow!
| ...blockStyle, | ||
| layout: { | ||
| ...layout, | ||
| flexSize: width || '72px', |
There was a problem hiding this comment.
Does this need to account for height and/or the spacing presets values? When dragging into a Stack variation, it looks like it defaults to the 72px value:
There was a problem hiding this comment.
Ah yes, well spotted! I've fixed the logic in the effect now and the problem with fill state should be addressed.
I suppose another possibility would be an attribute in |
Yeah, that's what I was thinking! I guess blocks with no intrinsic width/height are a bit of an edge case so it wouldn't be too cumbersome to do that, but are there any downsides to introducing this new API? |
There was a problem hiding this comment.
This is feeling closer to me! I ran into another issue while re-testing, but one thing I did notice is that now that Fit is never used as a default value, it doesn't seem to be as offputting that it results in 0 being applied. I think I'd probably lean toward leaving Fit in as an option for this PR, and we could look at reducing the available options potentially in a follow-up PR?
| const newSize = | ||
| inheritedOrientation === 'horizontal' | ||
| ? getSpacingPresetCssVar( width ) || '72px' | ||
| : getSpacingPresetCssVar( height ) || '100px'; |
There was a problem hiding this comment.
This seems to be working a bit better now, but it looks like there's still a couple of issues when dragging a Spacer block in and out of a flex container. Here's a quick gif:
I'm not too sure what's happening, unfortunately I've found it a little hard to figure out, but my observations so far are:
- The conversion from the spacing preset to the
pxvalue doesn't appear to populate the Fixed input field - When moving into the flex container block and out again, somewhere along the way, it seems that the size values become stale. So some of the time the value appears to be inconsistent (like it's remembering the one set in the flex state or vice-versa)
There was a problem hiding this comment.
The conversion from the spacing preset to the px value doesn't appear to populate the Fixed input field
It's not converting, so the input doesn't understand the value of the preset. Perhaps we should make it possible to set preset sizes in the child layout controls; I'm sure it would be useful outside of this scenario?
When moving into the flex container block and out again, somewhere along the way, it seems that the size values become stale
Oooh well spotted! We're alternately setting height and flexSize and not changing either of them when setting the other, so the block attributes end up something like: {"height":"var:preset|spacing|40","style":{"layout":{"flexSize":"130px","selfStretch":"fixed"}}}. I'll look into it!
There was a problem hiding this comment.
It's not converting
Ah, I think I'm following along now. I like the idea of adding support for presets for the child layout controls, but I wonder if we'd ultimately want a different scale for size presets than spacing, given that width and height values will often need to be much bigger than spacing between things?
For now, what do you think about potentially using getCustomValueFromPreset instead of getSpacingPresetCssVar so that when dragging from outside a Row / Stack block into one of them, we switch to using the real pixel value?
There was a problem hiding this comment.
I wonder if we'd ultimately want a different scale for size presets than spacing, given that width and height values will often need to be much bigger than spacing between things?
Yeah, that's a great point! I'll convert to px for now.
| }, [ | ||
| blockStyle, | ||
| flexSize, | ||
| height, | ||
| inheritedOrientation, | ||
| isFlexLayout, | ||
| layout, | ||
| selfStretch, | ||
| setAttributes, | ||
| width, | ||
| ] ); |
There was a problem hiding this comment.
Does this mean that the logic here will be called more frequently now? And if so, is that a safe change? From a quick skim it sounds like it's intentional for it to be called more frequently, but I wasn't sure if there's a potential state it could get in where setAttributes could result in the effect being run too often 🤔
There was a problem hiding this comment.
Yeah that's a good point, I'm not 100% sure there won't be nefarious side effects. I mainly added the dependencies in because the linter was complaining about it 😅 and it didn't seem to do any harm, but negative perf impacts often don't show immediately.
There was a problem hiding this comment.
Update: I tried removing them and ended up re-adding as the effect wasn't running when a Spacer changed to "fit" or "fill" and the old width/height value was still showing up when dragging outside of the flex container. It doesn't seem to hugely increase the times the effect runs though I might easily be missing some edge case situations.
There was a problem hiding this comment.
In practice, I don't think I've encountered any issues with this list of dependencies so far. It might just be something for us to keep an eye on if anyone runs into any issues with the spacer?
There was a problem hiding this comment.
This is testing really well, it feels very close to being mergeable to me! Just left a comment about whether we should swap from CSS variables to px when moving styles from width/height to flexSize.
The other thing I noticed is that when dragging into the flex container, the setAttributes call doesn't clear out the width or height attributes, so in the markup and on the site frontend, it's still outputting the inline style for width or height respectively. The flex-basis size winds up overriding it, but for tidiness, should we clear those values when the flexSize is updated?
Here's some block markup:
<!-- wp:spacer {"height":"223px","style":{"layout":{"flexSize":"78px","selfStretch":"fixed"}}} -->
<div style="height:223px" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->
What do you think? Thanks for all the back and forth — I don't think we need to get it perfect in this PR, so I reckon we could get pretty close and continue to tweak in follow-ups if that winds up being easier.
| const newSize = | ||
| inheritedOrientation === 'horizontal' | ||
| ? getSpacingPresetCssVar( width ) || '72px' | ||
| : getSpacingPresetCssVar( height ) || '100px'; |
There was a problem hiding this comment.
It's not converting
Ah, I think I'm following along now. I like the idea of adding support for presets for the child layout controls, but I wonder if we'd ultimately want a different scale for size presets than spacing, given that width and height values will often need to be much bigger than spacing between things?
For now, what do you think about potentially using getCustomValueFromPreset instead of getSpacingPresetCssVar so that when dragging from outside a Row / Stack block into one of them, we switch to using the real pixel value?
| }, [ | ||
| blockStyle, | ||
| flexSize, | ||
| height, | ||
| inheritedOrientation, | ||
| isFlexLayout, | ||
| layout, | ||
| selfStretch, | ||
| setAttributes, | ||
| width, | ||
| ] ); |
There was a problem hiding this comment.
In practice, I don't think I've encountered any issues with this list of dependencies so far. It might just be something for us to keep an eye on if anyone runs into any issues with the spacer?
|
Thanks for the latest round of testing @andrewserong ! I've updated the PR to change presets to unit values and remove unnecessary attributes when moving between containers. |
| @@ -430,6 +430,19 @@ _Returns_ | |||
|
|
|||
| - `string|null`: A font-size value using clamp(). | |||
|
|
|||
| ### getCustomValueFromPreset | |||
There was a problem hiding this comment.
I totally forgot that we're of course working in a different package when making changes to the Spacer block, so this function needs to be exported now.
Shall we rename it to getSpacingCustomValueFromPreset or getCustomSpacingValueFromPreset since it's being exported from the block editor package? That way if we have other functions for different kinds of presets the need to be exported in the future, we won't have a collision?
That said, from a quick search I see that the native version of index.native.js for the block editor package is already exporting this function as-is here: https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/index.native.js#L79.
So, perhaps another option would be to leave it as-is and if in the future we need a more generic getCustomValueFromPreset to be exported that handles other kinds of presets, then we could add a parent function that internally calls the existing spacing one.
What do you think?
There was a problem hiding this comment.
I'd leave it as is for now, and then perhaps if/when we need to handle other types of presets, we can reimplement this function to deal with any preset, so we don't have to change existing usage (we'd probably want to move it out of the spacing utils if we did that of course).
| flexSize: null, | ||
| selfStretch: null, |
There was a problem hiding this comment.
Nit: it looks like setting to null results in null being serialized to the block markup. Should these be undefined instead?
E.g. here's block markup after this is updated (it's similar with the height and width attributes above):
<!-- wp:spacer {"height":"clamp(7rem, 14vw, 11rem)","style":{"layout":{"flexSize":null,"selfStretch":null}}} -->
<div style="height:clamp(7rem, 14vw, 11rem)" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->There was a problem hiding this comment.
Oh whoops, I fixed that and then didn't push the change 😳
There was a problem hiding this comment.
This is so very close! Unfortunately, I've run into an odd block validation error and I can't quite work out what's causing it. To reproduce:
- Add a spacer block outside of a Stack block and set a custom height
- Drag the block into a Stack block
- Save the post
- Reload, and see that there is now a block validation error:
2023-04-18.14.57.22.mp4
Here's the error message:
Content generated by `save` function:
<div style="height:100px" aria-hidden="true" class="wp-block-spacer"></div>
Content retrieved from post body:
<div aria-hidden="true" class="wp-block-spacer"></div>
I'm wondering if it has something to do with setting height and width to undefined now? Since the height attributes's default value in block.json is 100px, could it be to do with when height is explicitly set to undefined versus when it simply has no value and defaults to 100px when the page is loaded?
I'm not quite sure what's going on there, but one potential fix might be for us to retain setting width and height to null when clearing them out (but leave setting the flex layout values to undefined 🤔
Yeah this is a funny one. The error only goes away when setting an explicit height value (null causes the same error in my testing) so I've changed it to Hopefully all is working as expected now 😅 |
There was a problem hiding this comment.
Okay, I think that's finally gotten to the bottom of the edge cases for me. Thanks so much again for your patience, and for all the back and forth! 🙇 I've left one tiny non-blocking nit, but this is all working well:
✅ Dragging from root/outside Row or Stack to within the Flex block retains the styling correctly
✅ When using presets that have a px value, that value is pre-populated in the Fixed input field
✅ Fit and Fill work as expected based on the discussion thus far, with known drawback for Fit, but it isn't a default value, so seems fine to retain in the UI for now
✅ For themes using presets that are non-px values, e.g. TT3 that uses clamp() rules, the value is still applied and works in the editor and on the site frontend, but the input field is empty. This is to be expected since the child layout controls don't yet support a presets slider or other kind of UI, and I think an empty field is the most graceful fallback we have at the moment 👍
✅ Default values work correctly when inserting a new Spacer block
✅ Spacer block works correctly outside of flex blocks
LGTM! ✨
| setAttributes( { | ||
| height: '0px', | ||
| width: '72px', | ||
| style: { |
There was a problem hiding this comment.
One final nit (and not a blocker), but I just noticed that sometimes the serialized block markup contains an empty layout object within style. We can use cleanEmptyObject to remove these, e.g. like in the Cover block here:
It doesn't cause any block validation issues to retain the empty objects in the block markup, though, so I really don't think it's a blocker.
There was a problem hiding this comment.
Funnily enough, running the object passed to setAttributes through cleanEmptyObject results in the layout attributes not being unset at all, I'm not sure why. I might leave it as is and avoid going down a rabbit hole with this 😅
There was a problem hiding this comment.
I might leave it as is and avoid going down a rabbit hole with this 😅
Good thinking! I'm in support of the idea of merging this now that it's in a good and working state, and we can look at further tweaks in isolation 👍
Actually, they do work (within a row/stack):
|
What?
Fixes #38022.
flex-basisvalue instead of width and height.Note: I've made the "Fit" control reset the block width/height to 0 instead of removing it altogether, as removing "Fit" presents a technical challenge, due to those controls not knowing about the blocks they're applied to. I'm not sure if it makes sense to try and hack around this because Spacer is probably the only block that has no intrinsic size.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast