Min Height: Add height control component with slider #45875
Conversation
andrewserong
added
[Type] Enhancement
|
|
|
Size Change: +806 B (0%) Total Size: 1.3 MB
ℹ️ View Unchanged
|
|
Things are working well for me!
I adjusted the After that I think the max properties make sense. It's hard to arrive at a figure, especially for Mentioned also in #44598 (comment), for me it's a little disconcerting to have entered a high value, e.g., Cheers! |
Co-authored-by: Ramon <ramonjd@users.noreply.github.com>
There was a problem hiding this comment.
Nice improvement @andrewserong ✨
✅ Works well in the post editor
✅ Works in the site editor
In terms of UX, it behaves and feels pretty much as expected. When switching units it can feel a bit clunky, as for example, setting a value of 30px and then changing the unit to em results in a huge jump.
In a follow-up, would it be worth exploring resetting the value to something suitable when switching units? It might be able to be handled by exposing the UnitControl's onUnitChange prop on the new HeightControl. I have a feeling this has been done elsewhere.
Another minor edge case in terms of UX would be the % units. In a lot of scenarios, this appears to have no visual effect.
The behaviour of the HeightControl where it won't limit the value entered into the UnitControl by a max value is something that isn't currently supported by the new slider controls under development. If this proves to be a common use case, we might need to consider adding an option for that into the proposed SliderNumbeControl and SliderUnitControl components.
| em: { max: 50, steps: 0.1 }, | ||
| rem: { max: 50, steps: 0.1 }, |
There was a problem hiding this comment.
Was it a deliberate choice to increase the step value for em and rem values from other controls (0.01)? The PR description explains the higher max values only.
There was a problem hiding this comment.
Oh, this one was also a copy + paste, but the intention of these custom step values is for it to only apply to the range control instead of the unit control. With the unit control it's important that the step value is small enough to support specific values that folks might use (so .25 is a useful fraction of an em or rem value for example). However, for the range control, I think it's valuable to use a slightly larger step value for the case of a user with the slider thumb focused pressing left/right keys. I've updated the PR description to include the following:
Note: higher step values are also included for the range control only. This allows pressing left/right arrows with the range control focused to have a more immediate visual effect on the minimum height in use. The higher values are only applied to the range control so that the unit control can still support more granular / precise values.
I've also renamed the constant so that it's hopefully a bit clearer that it only applies to the range control. Also, happy to go with different defaults if this feels off 🙂
| value={ customRangeValue } | ||
| min={ 0 } | ||
| max={ | ||
| CUSTOM_VALUE_SETTINGS[ selectedUnit ]?.max ?? 10 |
There was a problem hiding this comment.
What's the thinking behind a default max of 10?
There was a problem hiding this comment.
Oh, this one was just a copy+paste that I hadn't considered. From looking over some of the other possible unit types that aren't covered here, I think maybe a default max of 100 would be better for this component (for example if a theme allowed ch units). Happy to go with a different value if anyone can think of a better fallback max.
There was a problem hiding this comment.
A default of 100 was probably more what I was expecting as it's the default for our RangeControl I think.
| const selectedUnit = | ||
| useMemo( | ||
| () => parseQuantityAndUnitFromRawValue( value ), | ||
| [ value ] | ||
| )[ 1 ] || units[ 0 ].value; |
There was a problem hiding this comment.
From memory, I think it's possible we can get an empty units array. Should we guard against that edge case here?
There was a problem hiding this comment.
Good call, I've added a fallback px to this line.
|
Thanks for the reviews and feedback @ramonjd and @aaronrobertshaw! 🙇 In c8351eb I've had a go at adding in a handler for switching unit types. It isn't perfect, but seems to make the process of switching between
Thanks for flagging that — I feel like it's probably a little edge-casey in that for minimum height we do want to allow folks to set quite large values if they really want to, but in most other cases I imagine we probably don't. When starting this PR, I first thought that it mightn't be worth adding a whole separate component for height, but in practice it does seem like height / minimum height controls do need at least a little bit of their own logic for the unique case of dealing with quite big values. |
|
Update: I've added in a storybook example, too in 8cad9c7, which highlighted a couple of minor tweaks to make sure the component works well in isolation. It seems to be working pretty well now:
|
|
I think this PR is pretty close for a final(ish) review, so I've added a few more folks as reviewers who might be interested in this one. It isn't super urgent, though, so no worries if you're not able to look at this in the short term! 🙂 |
|
At a glance, seems okay to me, especially if it's a non default control. I think there are explorations worth having in the future, of how we can make the control apply smarter responsive-ready defaults. I imagine it could work a little bit like the font size control, with presets by default, and an advanced toggle that replaces it with an input + slider (as this PR adds). That could potentially let us make useful clamp based height presets to pick from. What do you think? |
I love that kind of idea, and I think whatever concept / controls or affordances we have for preset and clamp based rules would also be great as an addition to width controls (such as in #45364). As you say, the control in this PR is the advanced version and hidden by default, so the goal would be to land this PR first so that we've got the consistency of the design tool, and we could look at the preset-like / clampified approach separately. Because dealing with responsive / clamp-based rules and presets is likely a pretty big rabbithole to go down (because the presets would need to be different values to the existing spacing presets), if designers have time to investigate, do you mind following up in a separate issue to explore the potential options there? The spacing controls had a lot of back and forth, so there's probably quite a few things to nut out before we jump into code for that one. I'm very keen for us to come up with a good and intuitive approach for that, though! |
There was a problem hiding this comment.
Thanks for all the revisions @andrewserong
This is working well for me for the post content and group blocks (the two blocks that opt into min height).
Here's what I'm seeing:
2022-11-23.15.33.50.mp4
And I like!
|
Thanks for the review @ramonjd! I'll wait until tomorrow before merging to see if there are any blocking comments, and if not, will merge this in then. Also happy to continue to chip away at min height controls in follow-ups, too! |
What?
Part of #45501 and a follow-up to #45300
Add a
HeightControlcomponent to the block editor package, to be used for the Minimum Height control. It pairs aUnitControland aRangeControltogether, along with some default maximum values for units, which are suitable for the height control. These values are necessarily much higher than those used for the spacing controls like padding and margin, as it's likely that folks will be setting larger values for minimum heights.Note: higher
stepvalues are also included for the range control only. This allows pressing left/right arrows with the range control focused to have a more immediate visual effect on the minimum height in use. The higher values are only applied to the range control so that the unit control can still support more granular / precise values.Why?
For consistency with design tools, and to implement the feedback as raised in #45300 (comment) — also, a slider control will be easier for folks to interact with to see immediate adjustments to height values.
How?
HeightControlcomponent that pairs aUnitControlwith aRangeControlcomponent, and that provides default maximum values for the slider. Use this new component at both the individual block level and within global styles, for the minimum height control.Testing Instructions
px,em, orremvalues?)You can also test out the included storybook example by running
npm run storybook:devlocally, and then navigating to: http://localhost:50240/?path=/story/blockeditor-heightcontrol--defaultScreenshots or screencast