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

NumberControl: Add custom spin buttons #45333

Merged
merged 15 commits into from
Nov 2, 2022
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
- `UnitControl`: Remove outer wrapper to normalize className placement ([#41860](https://github.com/WordPress/gutenberg/pull/41860)).
- `ColorPalette`: Fix transparent checkered background pattern ([#45295](https://github.com/WordPress/gutenberg/pull/45295)).
- `ToggleGroupControl`: Add `isDeselectable` prop to allow deselecting the selected option ([#45123](https://github.com/WordPress/gutenberg/pull/45123)).
- `NumberControl`: Add custom spin buttons ([#45333](https://github.com/WordPress/gutenberg/pull/45333)).

### Bug Fix

Expand Down
60 changes: 56 additions & 4 deletions packages/components/src/number-control/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,28 @@
* External dependencies
*/
import classNames from 'classnames';
import type { ForwardedRef } from 'react';
import type { ForwardedRef, ChangeEvent } from 'react';

/**
* WordPress dependencies
*/
import { forwardRef } from '@wordpress/element';
import { isRTL } from '@wordpress/i18n';
import { isRTL, __ } from '@wordpress/i18n';
import { plus as plusIcon, reset as resetIcon } from '@wordpress/icons';

/**
* Internal dependencies
*/
import { Input } from './styles/number-control-styles';
import { Input, SpinButton } from './styles/number-control-styles';
import * as inputControlActionTypes from '../input-control/reducer/actions';
import { add, subtract, roundClamp } from '../utils/math';
import { ensureNumber, isValueEmpty } from '../utils/values';
import type { WordPressComponentProps } from '../ui/context/wordpress-component';
import type { NumberControlProps } from './types';
import { HStack } from '../h-stack';
import { Spacer } from '../spacer';

const noop = () => {};

function UnforwardedNumberControl(
{
Expand All @@ -36,6 +41,9 @@ function UnforwardedNumberControl(
step = 1,
type: typeProp = 'number',
value: valueProp,
size = 'default',
suffix,
onChange = noop,
...props
}: WordPressComponentProps< NumberControlProps, 'input', false >,
ref: ForwardedRef< any >
Expand Down Expand Up @@ -178,14 +186,27 @@ function UnforwardedNumberControl(
return nextState;
};

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 } );
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did try a few different approaches here but couldn't figure out anything that felt right.

  • We can't dispatch reducer actions from NumberControl as the dispatch function in InputControl isn't available.
  • We can't call the reducer manually in NumberControl as the existing reducer state in InputControl isn't available.
  • I don't think it makes sense for InputControl to have the spin button functionality since it is supposed to be a generic input. (The docs say it's intended to be an eventual TextControl replacement.)

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I don't think it makes sense for InputControl to have the spin button functionality since it is supposed to be a generic input. (The docs say it's intended to be an eventual TextControl replacement.)

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?

    setIsValidValue( extra.event.target.validity.valid );

  • 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?

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@ciampo ciampo Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 of NumberControl
  • I remember discussing whether the isPressEnterToChange prop is necessary at all

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)


return (
<Input
autoComplete={ autoComplete }
inputMode="numeric"
{ ...props }
className={ classes }
dragDirection={ dragDirection }
hideHTMLArrows={ hideHTMLArrows }
hideHTMLArrows={ size === '__unstable-large' || hideHTMLArrows }
isDragEnabled={ isDragEnabled }
label={ label }
max={ max }
Expand All @@ -200,6 +221,37 @@ function UnforwardedNumberControl(
const baseState = numberControlStateReducer( state, action );
return stateReducerProp?.( baseState, action ) ?? baseState;
} }
size={ size }
suffix={
size === '__unstable-large' && ! hideHTMLArrows ? (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@noisysocks noisysocks Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A spinControls prop would've been perfect if we actually had custom spin controls for the other size variants 😂

Switching hideHTMLArrows to true by default works for me 👍

Cool. @ciampo ^ We'll go with this if you don't have any objections.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me — just to make sure, we will still show these custom, larger spin buttons only for the __unstable-large size variant?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that makes sense 👍 I really appreciate the effort and care you put into working out the best solution!

<>
{ suffix }
<Spacer marginBottom={ 0 } marginRight={ 2 }>
<HStack spacing={ 1 }>
<SpinButton
icon={ plusIcon }
isSmall
aria-hidden="true"
aria-label={ __( 'Increment' ) }
tabIndex={ -1 }
onClick={ buildSpinHandler( 'up' ) }
/>
<SpinButton
icon={ resetIcon }
isSmall
aria-hidden="true"
aria-label={ __( 'Decrement' ) }
tabIndex={ -1 }
onClick={ buildSpinHandler( 'down' ) }
/>
</HStack>
</Spacer>
</>
) : (
suffix
)
}
onChange={ onChange }
/>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,13 @@
*/
import { css } from '@emotion/react';
import styled from '@emotion/styled';

/**
* Internal dependencies
*/
import InputControl from '../../input-control';
import { COLORS } from '../../utils';
import Button from '../../button';

const htmlArrowStyles = ( { hideHTMLArrows } ) => {
if ( ! hideHTMLArrows ) return ``;
Expand All @@ -28,3 +31,9 @@ const htmlArrowStyles = ( { hideHTMLArrows } ) => {
export const Input = styled( InputControl )`
${ htmlArrowStyles };
`;

export const SpinButton = styled( Button )`
&&& {
color: ${ COLORS.ui.theme };
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

`;
51 changes: 51 additions & 0 deletions packages/components/src/number-control/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* External dependencies
*/
import { render, screen, fireEvent, waitFor } from '@testing-library/react';
import userEvent from '@testing-library/user-event';

/**
* WordPress dependencies
Expand Down Expand Up @@ -425,4 +426,54 @@ describe( 'NumberControl', () => {
expect( input.value ).toBe( '4' );
} );
} );

describe( 'custom spin buttons', () => {
test.each( [
{},
{ size: 'small' },
{ size: '__unstable-large', hideHTMLArrows: true },
] )( 'should not appear when props = %o', ( props ) => {
render( <NumberControl { ...props } /> );
expect(
screen.queryByLabelText( 'Increment' )
).not.toBeInTheDocument();
expect(
screen.queryByLabelText( 'Decrement' )
).not.toBeInTheDocument();
} );

test.each( [
[ 'up', '1', {} ],
[ 'up', '2', { value: '1' } ],
[ 'up', '12', { value: '10', step: '2' } ],
[ 'up', '10', { value: '10', max: '10' } ],
[ 'down', '-1', {} ],
[ 'down', '1', { value: '2' } ],
[ 'down', '10', { value: '12', step: '2' } ],
[ 'down', '10', { value: '10', min: '10' } ],
] )(
'should spin %s to %s when props = %o',
async ( direction, expectedValue, props ) => {
const user = userEvent.setup( {
advanceTimers: jest.advanceTimersByTime,
} );
const onChange = jest.fn();
render(
<NumberControl
{ ...props }
size="__unstable-large"
onChange={ onChange }
/>
);
await user.click(
screen.getByLabelText(
direction === 'up' ? 'Increment' : 'Decrement'
)
);
expect( onChange ).toHaveBeenCalledWith( expectedValue, {
event: expect.anything(),
} );
}
);
} );
} );