NumberControl: Add custom spin buttons #45333
Conversation
noisysocks
added
[Type] Enhancement
noisysocks
added
the
Global Styles
|
Size Change: +348 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
|
This is looking and working well for me. 🍠 ✅ Mouse interactions with plus/minus buttons work as expected |
|
Thanks @ramonjd! I added unit tests and updated the changelog. Probably best to page @ciampo and @jasmussen for extra eyes. |
|
I know that @mirka was involved in some conversations around this, so probably better to wait for her feedback here |
| const buildSpinHandler = | ||
| ( direction: 'up' | 'down' ) => | ||
| ( event: ChangeEvent< HTMLInputElement > ) => { | ||
| let nextValue = isValueEmpty( valueProp ) ? baseValue : valueProp; | ||
| if ( direction === 'up' ) { | ||
| nextValue = add( nextValue, baseStep ); | ||
| } else if ( direction === 'down' ) { | ||
| nextValue = subtract( nextValue, baseStep ); | ||
| } | ||
| nextValue = constrainValue( nextValue ); | ||
| onChange( String( nextValue ), { event } ); | ||
| }; |
There was a problem hiding this comment.
Not a blocker for this PR to land, but have you considered hooking these buttons up to the reducer system that is already in place? Having the buttons dispatch the pressUp/pressDown actions would dedupe the logic and would also add the Shift key enhancement.
I'm guessing that would involve moving the spin buttons down into the InputControl component, and some re-plumbing. Unclear whether it's worth the effort though. cc @stokesman and @ciampo who I'm sure have stronger ✨ feelings ✨ about the reducer construct.
There was a problem hiding this comment.
I did try a few different approaches here but couldn't figure out anything that felt right.
- We can't dispatch reducer actions from
NumberControlas thedispatchfunction inInputControlisn't available. - We can't call the reducer manually in
NumberControlas the existing reducer state inInputControlisn't available. - I don't think it makes sense for
InputControlto have the spin button functionality since it is supposed to be a generic input. (The docs say it's intended to be an eventualTextControlreplacement.)
So I ended up settling on this boring repetitive approach. I can DRY it up a bit further by making the reducer and spin buttons use the same logic. I've done that in e63d7ed.
Let me know if you have any other suggestions.
There was a problem hiding this comment.
- I don't think it makes sense for
InputControlto have the spin button functionality since it is supposed to be a generic input. (The docs say it's intended to be an eventualTextControlreplacement.)
This is the thing — InputControl already contains a lot of the scaffolding that seems generic but basically only makes sense for number inputs (drag gestures, arrow key handlers). One can argue that spin buttons are just as generic as up/down arrow key handlers are. I guess in its current state, it's really a <input type="potentially anything, extend me!"> control rather than a strict <input type="text"> control.
Some other things I'm thinking in relation to this:
-
This line in Storybook will error when using the big spin buttons. Is that a problem?
-
The
isPressEnterToChangeprop currently does not behave as expected when drag gestures and up/down keys are involved. When we fix this logic at some point, will we also have to fix the big spin buttons separately?
So, on the surface it kind of seems like things will generally be simpler if make the big spin buttons "generic" (wink wink) and move them down into InputControl.
But in any case, I think this PR is DRY enough for the time being, and it wouldn't be hard to extract the buttons if we feel like it in the future. (We should look at that event.target.validity.valid thing before merging though)
There was a problem hiding this comment.
InputControl already contains a lot of the scaffolding that seems generic but basically only makes sense for number inputs (drag gestures, arrow key handlers).
It does yet it seems wrong to me. Instead of putting any more effort into fitting that mold my feeling is InputControl ought to be broken into a hook + component combo. That would allow NumberControl to use the hook and get direct access to dispatch for more flexible extensibility. It would also allow making InputControl more generic. I have an old experimental branch doing that and I'd been thinking to make a fresh start on it after NumberControl was converted to TS. Now, I'm not sure when I'll find the time to do so.
There was a problem hiding this comment.
Not necessarily related to the immediate changes that this PR may need to make in order to be merged relatively soon — but my general feeling is also that InputControl could/should be simplified:
- the reducer logic seems complicated and potentially over-engineered, I'd love to see if we can refactor it to be simpler
- a lot of number-related functionality is written in
InputControl, while it would make sense to me if they were part ofNumberControl - I remember discussing whether the
isPressEnterToChangeprop is necessary at all
There was a problem hiding this comment.
I see 😅 thanks for the discussion.
It's easier to move the custom spinners from NumberControl to InputControl later in a BC way than it is to do the opposite, so I'm personally inclined to stick with putting them in NumberControl at least for now.
This line in Storybook will error when using the big spin buttons. Is that a problem?
Good catch. I'll fix this by overriding event.target to be the <input>. We do this already for the drag events.
The isPressEnterToChange prop currently does not behave as expected when drag gestures and up/down keys are involved. When we fix this logic at some point, will we also have to fix the big spin buttons separately?
I would expect that setting isPressEnterToChange should only make it so that onChange is not called while the user is typing a number and should not have any effect on the spin controls, no?
the reducer logic seems complicated and potentially over-engineered, I'd love to see if we can refactor it to be simpler
Yeah not a huge fan of this reducer pattern. I don't really see why InputControl couldn't be a standard managed component with callback props for each of the actions (onPressDown, onPressUp, etc.)
| &&& { | ||
| color: ${ COLORS.ui.theme }; | ||
| } |
There was a problem hiding this comment.
I wonder how common this styling is going to be. A similar one is the unit dropdown in UnitControl. Do you happen to know any others? Hoping this will remain a one-off 😂
There was a problem hiding this comment.
I'm not sure, sorry!
It's very possible I misunderstood the design and the + / - buttons are supposed to be tertiary (link) buttons and not regular buttons that have the theme's accent colour. Do you know @jasmussen?
| @@ -200,6 +221,37 @@ function UnforwardedNumberControl( | |||
| const baseState = numberControlStateReducer( state, action ); | |||
| return stateReducerProp?.( baseState, action ) ?? baseState; | |||
| } } | |||
| size={ size } | |||
| suffix={ | |||
| size === '__unstable-large' && ! hideHTMLArrows ? ( | |||
There was a problem hiding this comment.
The big thing we need to decide before merging is this part!
While I do think the current logic makes sense, I also feel like the big spin buttons should be disabled by default 🤔 They take up a lot of space, so I'm afraid many use cases would prefer to disable them unless they were particularly useful for the specific case. On the other hand, this API could get super messy with the hideHTMLArrows and the size variants.
The middle ground I can think of is to update the API so hideHTMLArrows is true by default. I think this is a palatable change, given that NumberControl is still experimental and the visual change is non-disruptive. What do you all think?
There was a problem hiding this comment.
Switching hideHTMLArrows to true by default works for me 👍
Another option is to remove hideHTMLArrows in favour of something like spinControls: 'hidden' | 'native' | 'custom'.
Let me know—happy to defer to you and @ciampo.
There was a problem hiding this comment.
A spinControls prop would've been perfect if we actually had custom spin controls for the other size variants 😂
Switching
hideHTMLArrowstotrueby default works for me 👍
Cool. @ciampo ^ We'll go with this if you don't have any objections.
There was a problem hiding this comment.
Sounds good to me — just to make sure, we will still show these custom, larger spin buttons only for the __unstable-large size variant?
There was a problem hiding this comment.
I changed my mind 😅
I started working on this and in the process appreciated how many instances of NumberControl we have in the block editor (quite a few!) and how, in all instances, the arrow buttons / spin buttons are quite handy. I fear if we make hideHTMLArrows true by default that developers will forget to set hideHTMLArrows={ false } and that the UX will suffer. In other words I really think showing arrow buttons / spin buttons is a sensible default.
So I went ahead and implemented a spinControls prop as above. The default is spinControls = 'native'. I also added some styling so that if spinControls = 'custom' and size = 'small' things look squished but not totally broken.
I think this makes sense. Let me know if you disagree!
There was a problem hiding this comment.
Yeah, that makes sense 👍 I really appreciate the effort and care you put into working out the best solution!
Typing the event attribute as PointerEvent<T> | ChangeEvent<T> isn't great as it means that future ways to input a value into InputControl, NumberControl and UnitControl will result in BC breaking type changes. Consumers of the component don't need to know the specifics of the event beyond that it's a synthetic event and can use `is` to determine more if necessary.
| export type InputChangeCallback< P = {} > = ( | ||
| nextValue: string | undefined, | ||
| extra: { event: SyntheticEvent } & P | ||
| ) => void; |
There was a problem hiding this comment.
I changed InputControl's onChange callback type to be less opinionated.
Typing the event attribute as PointerEvent<T> | ChangeEvent<T> isn't great for maintainability as it means every time a new way to input a value is added to InputControl, NumberControl or UnitControl (which is what this PR does) then there will be a BC-breaking type change.
Consumers of the component don't need to know the specifics of the event beyond that it's a synthetic event (not a native event). Consumers can use is to determine the event type if necessary.
|
|
||
| function UnforwardedNumberControl( | ||
| { | ||
| __unstableStateReducer: stateReducerProp, | ||
| className, | ||
| dragDirection = 'n', | ||
| hideHTMLArrows = false, |
There was a problem hiding this comment.
We aren't bound to this by contract because NumberControl is marked as experimental, but it would be trivial to add a deprecated() and a line of back compat logic. I don't feel strongly about it either way, but just mentioning in case it wasn't considered.
There was a problem hiding this comment.
I was wondering this 🙂 I'll add a deprecated(), costs nothing!
What?
Adds custom spin buttons (+ / - buttons) to
NumberControl.Why?
So that controls such as Line height in Global Styles match the design, see #34345 (comment).
How?
suffixslot thatInputControlhas.InputControlor using the context system, but usingsuffixworks really well and requires minimal changes.valuebystep. Values are clamped to be withinminandmax.'default'and'small'sizes as these sizes don't have enough height to fit custom buttons.Testing Instructions
Check out the story book for
NumberControlor go to Appearance → Editor → Global Styles → Typography → Text.Screenshots or screencast
Kapture.2022-10-28.at.16.35.52.mp4