UnitControl: Use hideHTMLArrows prop to hide spin buttons
#43985
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: -1.62 kB (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
mirka
commented
Sep 8, 2022
| @@ -35,7 +35,6 @@ export default function BoxUnitControl( { | |||
| <UnitControl | |||
| aria-label={ label } | |||
| className="component-box-control__unit-control" | |||
| hideHTMLArrows | |||
There was a problem hiding this comment.
In BoxControl, spin buttons were always hidden, even when units were disabled. (Units get disabled in the "linked" control when the units of the separate box sides are "mixed".)
However, other BoxControl-like controls in the block editor like the Spacing and Border Radius controls use a raw UnitControl and do not hide spin buttons when units are disabled.
@jasmussen Any opinions on whether we should unify these hiding behaviors in the UnitControls? The options are:
- In a UnitControl, always show spin buttons when the units are disabled.
- In a UnitControl, always hide spin buttons, regardless of whether units are disabled.
- The consumers of UnitControl should be able to choose whether spin button are hidden. (In this case, we'll also want to iron out the inconsistencies in the BoxControl-like controls I described above.)
There was a problem hiding this comment.
Just to be sure I understand you right, when you refer to spin buttons, you refer to these?
If yes, then I haven't seen a case where we need to/should surface these in the editor. We have a click & drag behavior:
There's also up arrow for incrementing, down arrow for decrementing, and shift up/down for doing that in increments of ten. Those may be less discoverable, but feel far more ergonomic to use than the tiny little hit areas of the up/down arrows. What do you think?
There was a problem hiding this comment.
IMO, in general showing spin buttons would be a good idea and would improve the input's usability. Spin buttons would offer a very "visually explicit" alternative to increase/decrease the value in the input field (while every other alternative mentioned is more implicit and hard to discover).
Any opinions on whether we should unify these hiding behaviors in the UnitControls? The options are:
- In a UnitControl, always show spin buttons when the units are disabled.
- In a UnitControl, always hide spin buttons, regardless of whether units are disabled.
- The consumers of UnitControl should be able to choose whether spin button are hidden. (In this case, we'll also want to iron out the inconsistencies in the BoxControl-like controls I described above.)
I would vote for 1 or 3.
Units get disabled in the "linked" control when the units of the separate box sides are "mixed"
Just to understand — in that case, BoxControl seems to show the word "Mixed", and so I'm not sure how spin buttons would be useful in this scenario?
There was a problem hiding this comment.
Given how small the buttons are, and the fact that you can at any one point type in an explicit value, the spin buttons really don't seem worth optimizing for.
There was a problem hiding this comment.
My original intent was to discuss step buttons in UnitControl only, but I guess now we're discussing whether to nix step buttons in general, including normal NumberControls.
I just remembered that we have a huge move in the other direction, waiting on our todo list to implement:
And IIRC, @mtias wrote somewhere that UnitControl was originally intended to be accompanied by a slider, like so:
So perhaps the unifying idea is that number inputs should have some sort of visually obvious auxiliary control, that is usable on a touch device. In this case, the tiny built-in step buttons do not satisfy this requirement anyway.
tl;dr — I guess I'm fine with removing the step buttons from UnitControl right now. And they'll also be removed from NumberControl once the large custom step buttons are implemented.
There was a problem hiding this comment.
Indeed, the big + – is meant to cover incremental steps where each step is meaningful, like line height, while the slider is meant to cover wider ranges. A + – input should not be paired with a slider; custom font-size really need to be paired with the slider.
There was a problem hiding this comment.
In case where UnitControl will need to be adjusted with small incremental steps, would we show the big "+" / "-" buttons next to the unit dropdown?
There was a problem hiding this comment.
What's the unit dropdown? + / – should show inside the control like the line height example.
There was a problem hiding this comment.
What's the unit dropdown?
The unit dropdown in the unit control component is the dropdown that is shown when clicking on the unit (e.g. "px"):
- / – should show inside the control like the line height example.
The line height control doesn't show a unit, and so it would be using a NumberControl.
My question was about instances in which a UnitControl is used (which means that the unit dropdown is shown), as was also pointed out by @mirka at the start of the conversation
My original intent was to discuss step buttons in UnitControl only
There was a problem hiding this comment.
Ah, I hear you know. Indeed, the idea is the + / – makes sense for unit-less controls. UnitControl should just work with the slider. I cannot think of a case where we'd have the + / – and unit right now.
Also fixes the `disableUnits` case in Firefox
mirka
force-pushed
the
fix/unit-control-spin-buttons
branch
from
ca37b82
to
fa3a151
Compare
|
I updated this PR so it always hides the spin button in a UnitControl. Out of the paths we can take, this is the simpler solution in anticipation of more ergonomic auxiliary controls being added in the near future. |
ciampo
reviewed
Sep 14, 2022
There was a problem hiding this comment.
Apart from some snapshots that need updating, code changes LGTM!
I'll defer to @jasmussen or @mtias for final approval on the "strategy"
|
Seems fine to me! The slider and plus/minus buttons are the ergonomic replacements, but I wouldn't worry too much about the interim. |
ciampo
approved these changes
Sep 14, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Uses NumberControl's
hideHTMLArrowsprop to hide the spin buttons, instead of custom CSS.Why?
While going through the NumberControl types (#43791), I noticed that UnitControl was adding its own CSS styles to hide the spin buttons, when in fact NumberControl provided that option via the
hideHTMLArrowsprop.How?
Remove the CSS overrides and use the designated prop.
Testing Instructions
npm run storybook:devand see the UnitControl story.disableUnitis true. Check Chrome/Firefox/Safari.