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.

Sign up for GitHub

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

NumberControl: Add custom spin buttons #45333

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
7b790e0
Rough pass at adding custom spin buttons to NumberControl
noisysocks Oct 27, 2022
016f253
Adjust spacing of step buttons to match design
noisysocks Oct 27, 2022
4938546
Pass along suffix if provided
noisysocks Oct 27, 2022
2cc5953
DRY up onClick handlers
noisysocks Oct 27, 2022
80335a2
Remove spin buttons from tab order
noisysocks Oct 27, 2022
f15a653
Add unit tests
noisysocks Oct 28, 2022
e5967b3
Update changelog
noisysocks Oct 28, 2022
50c011e
Merge branch 'trunk' into add/spin-buttons-to-number-control
noisysocks Oct 28, 2022
e63d7ed
Make reducer and spin buttons use same logic
noisysocks Oct 31, 2022
d1e2a98
Make InputControl's onChange callback type less strict
noisysocks Oct 31, 2022
c056f5c
Fake event.target so that event.target.validity works
noisysocks Nov 1, 2022
393218c
Replace hideHTMLArrows with spinControls prop
noisysocks Nov 1, 2022
6120b3b
Update CHANGELOG.md
noisysocks Nov 1, 2022
d1cc602
Use custom spin controls for Line Height
noisysocks Nov 1, 2022
6e9be91
Deprecate hideHTMLArrows instead of removing it
noisysocks Nov 1, 2022
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
View file
Open in desktop
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
10 changes: 4 additions & 6 deletions packages/components/src/input-control/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@
import type {
CSSProperties,
ReactNode,
ChangeEvent,
SyntheticEvent,
PointerEvent,
HTMLInputTypeAttribute,
} from 'react';
import type { useDrag } from '@use-gesture/react';
Expand Down Expand Up @@ -62,10 +60,10 @@ interface BaseProps {
size?: Size;
}

export type InputChangeCallback<
E = ChangeEvent< HTMLInputElement > | PointerEvent< HTMLInputElement >,
P = {}
> = ( nextValue: string | undefined, extra: { event: E } & P ) => void;
export type InputChangeCallback< P = {} > = (
nextValue: string | undefined,
extra: { event: SyntheticEvent } & P
) => void;
Copy link
Member Author

@noisysocks noisysocks Oct 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.


export interface InputFieldProps extends BaseProps {
/**
Expand Down
120 changes: 90 additions & 30 deletions packages/components/src/number-control/index.tsx
View file
Open in desktop
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, KeyboardEvent, MouseEvent } 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 All @@ -56,6 +64,23 @@ function UnforwardedNumberControl(
const autoComplete = typeProp === 'number' ? 'off' : undefined;
const classes = classNames( 'components-number-control', className );

const spinValue = (
value: string | number | undefined,
direction: 'up' | 'down',
event: KeyboardEvent | MouseEvent | undefined
) => {
event?.preventDefault();
const shift = event?.shiftKey && isShiftStepEnabled;
const delta = shift ? ensureNumber( shiftStep ) * baseStep : baseStep;
let nextValue = isValueEmpty( value ) ? baseValue : value;
if ( direction === 'up' ) {
nextValue = add( nextValue, delta );
} else if ( direction === 'down' ) {
nextValue = subtract( nextValue, delta );
}
return constrainValue( nextValue, shift ? delta : undefined );
};

/**
* "Middleware" function that intercepts updates from InputControl.
* This allows us to tap into actions to transform the (next) state for
Expand All @@ -78,33 +103,11 @@ function UnforwardedNumberControl(
type === inputControlActionTypes.PRESS_UP ||
type === inputControlActionTypes.PRESS_DOWN
) {
const enableShift =
( event as KeyboardEvent | undefined )?.shiftKey &&
isShiftStepEnabled;

const incrementalValue = enableShift
? ensureNumber( shiftStep ) * baseStep
: baseStep;
let nextValue = isValueEmpty( currentValue )
? baseValue
: currentValue;

if ( event?.preventDefault ) {
event.preventDefault();
}

if ( type === inputControlActionTypes.PRESS_UP ) {
nextValue = add( nextValue, incrementalValue );
}

if ( type === inputControlActionTypes.PRESS_DOWN ) {
nextValue = subtract( nextValue, incrementalValue );
}

// @ts-expect-error TODO: Resolve discrepancy between `value` types in InputControl based components
nextState.value = constrainValue(
nextValue,
enableShift ? incrementalValue : undefined
nextState.value = spinValue(
currentValue,
type === inputControlActionTypes.PRESS_UP ? 'up' : 'down',
event as KeyboardEvent | undefined
);
}

Expand Down Expand Up @@ -185,7 +188,7 @@ function UnforwardedNumberControl(
{ ...props }
className={ classes }
dragDirection={ dragDirection }
hideHTMLArrows={ hideHTMLArrows }
hideHTMLArrows={ size === '__unstable-large' || hideHTMLArrows }
isDragEnabled={ isDragEnabled }
label={ label }
max={ max }
Expand All @@ -200,6 +203,63 @@ 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 •
edited
Loading

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={ (
event: MouseEvent< HTMLButtonElement >
) => {
onChange(
String(
spinValue(
valueProp,
'up',
event
)
),
{ event }
);
} }
/>
<SpinButton
icon={ resetIcon }
isSmall
aria-hidden="true"
aria-label={ __( 'Decrement' ) }
tabIndex={ -1 }
onClick={ (
event: MouseEvent< HTMLButtonElement >
) => {
onChange(
String(
spinValue(
valueProp,
'down',
event
)
),
{ event }
);
} }
/>
</HStack>
</Spacer>
</>
) : (
suffix
)
}
onChange={ onChange }
/>
);
}
Expand Down
9 changes: 9 additions & 0 deletions packages/components/src/number-control/styles/number-control-styles.js
View file
Open in desktop
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
View file
Open in desktop
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(),
} );
}
);
} );
} );
6 changes: 1 addition & 5 deletions packages/components/src/unit-control/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ import type {
KeyboardEvent,
ForwardedRef,
SyntheticEvent,
ChangeEvent,
PointerEvent,
} from 'react';
import classnames from 'classnames';

Expand Down Expand Up @@ -115,9 +113,7 @@ function UnforwardedUnitControl(
const handleOnQuantityChange = (
nextQuantityValue: number | string | undefined,
changeProps: {
event:
| ChangeEvent< HTMLInputElement >
| PointerEvent< HTMLInputElement >;
event: SyntheticEvent;
}
) => {
if (
Expand Down
9 changes: 4 additions & 5 deletions packages/components/src/unit-control/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import type { FocusEventHandler, SyntheticEvent } from 'react';
import type { FocusEventHandler } from 'react';

/**
* Internal dependencies
Expand Down Expand Up @@ -38,10 +38,9 @@ export type WPUnitControlUnit = {
step?: number;
};

export type UnitControlOnChangeCallback = InputChangeCallback<
SyntheticEvent< HTMLSelectElement | HTMLInputElement >,
{ data?: WPUnitControlUnit }
>;
export type UnitControlOnChangeCallback = InputChangeCallback< {
data?: WPUnitControlUnit;
} >;

export type UnitSelectControlProps = Pick< InputControlProps, 'size' > & {
/**
Expand Down