From d3544f925a5d1e0c205d89c666239468078b1d72 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Tue, 8 Mar 2022 18:36:51 -0800 Subject: [PATCH 01/19] Use a simpler way to allow changing units from the text input --- .../components/src/unit-control/index.tsx | 117 ++++-------------- .../src/unit-control/unit-select-control.tsx | 30 +++-- 2 files changed, 44 insertions(+), 103 deletions(-) diff --git a/packages/components/src/unit-control/index.tsx b/packages/components/src/unit-control/index.tsx index 5efb4aacfd0e0..c8dd72ba45c0a 100644 --- a/packages/components/src/unit-control/index.tsx +++ b/packages/components/src/unit-control/index.tsx @@ -1,12 +1,7 @@ /** * External dependencies */ -import type { - FocusEventHandler, - KeyboardEvent, - ForwardedRef, - SyntheticEvent, -} from 'react'; +import type { KeyboardEvent, ForwardedRef, SyntheticEvent } from 'react'; import classnames from 'classnames'; /** @@ -20,7 +15,6 @@ import { __ } from '@wordpress/i18n'; * Internal dependencies */ import type { WordPressComponentProps } from '../ui/context'; -import * as inputControlActionTypes from '../input-control/reducer/actions'; import { ValueInput } from './styles/unit-control-styles'; import UnitSelectControl from './unit-select-control'; import { @@ -31,7 +25,6 @@ import { } from './utils'; import { useControlledState } from '../utils/hooks'; import type { UnitControlProps, UnitControlOnChangeCallback } from './types'; -import type { StateReducer } from '../input-control/reducer/state'; function UnforwardedUnitControl( unitControlProps: WordPressComponentProps< @@ -59,7 +52,6 @@ function UnforwardedUnitControl( unit: unitProp, units: unitsProp = CSS_UNITS, value: valueProp, - onBlur: onBlurProp, onFocus: onFocusProp, ...props } = unitControlProps; @@ -76,10 +68,18 @@ function UnforwardedUnitControl( // ensures it fallback to `undefined` in case a consumer of `UnitControl` // still passes `null` as a `value`. const nonNullValueProp = valueProp ?? undefined; - const units = useMemo( - () => getUnitsWithCurrentUnit( nonNullValueProp, unitProp, unitsProp ), - [ nonNullValueProp, unitProp, unitsProp ] - ); + const [ units, reFirstCharacterOfUnits ] = useMemo( () => { + const list = getUnitsWithCurrentUnit( + nonNullValueProp, + unitProp, + unitsProp + ); + const firstCharacters = list.reduce( ( carry, { value } ) => { + const first = value.substr( 0, 1 ); + return carry.includes( first ) ? carry : `${ carry }|${ first }`; + }, list[ 0 ].value.substr( 0, 1 ) ); + return [ list, new RegExp( `^(?:${ firstCharacters })$` ) ]; + }, [ nonNullValueProp, unitProp, unitsProp ] ); const [ parsedQuantity, parsedUnit ] = getParsedQuantityAndUnit( nonNullValueProp, unitProp, @@ -100,9 +100,6 @@ function UnforwardedUnitControl( } }, [ parsedUnit, setUnit ] ); - // Stores parsed value for hand-off in state reducer. - const refParsedQuantity = useRef< number | undefined >( undefined ); - const classes = classnames( 'components-unit-control', // This class is added for legacy purposes to maintain it on the outer @@ -158,85 +155,22 @@ function UnforwardedUnitControl( setUnit( nextUnitValue ); }; - const mayUpdateUnit = ( event: SyntheticEvent< HTMLInputElement > ) => { - if ( ! isNaN( Number( event.currentTarget.value ) ) ) { - refParsedQuantity.current = undefined; - return; - } - const [ validParsedQuantity, validParsedUnit ] = - getValidParsedQuantityAndUnit( - event.currentTarget.value, - units, - parsedQuantity, - unit - ); - - refParsedQuantity.current = validParsedQuantity; - - if ( isPressEnterToChange && validParsedUnit !== unit ) { - const data = Array.isArray( units ) - ? units.find( ( option ) => option.value === validParsedUnit ) - : undefined; - const changeProps = { event, data }; - - // The `onChange` callback already gets called, no need to call it explicitly. - onUnitChange?.( validParsedUnit, changeProps ); - - setUnit( validParsedUnit ); - } - }; - - const handleOnBlur: FocusEventHandler< HTMLInputElement > = ( event ) => { - mayUpdateUnit( event ); - onBlurProp?.( event ); - }; - - const handleOnKeyDown = ( event: KeyboardEvent< HTMLInputElement > ) => { - const { key } = event; - if ( key === 'Enter' ) { - mayUpdateUnit( event ); - } - }; - - /** - * "Middleware" function that intercepts updates from InputControl. - * This allows us to tap into actions to transform the (next) state for - * InputControl. - * - * @param state State from InputControl - * @param action Action triggering state change - * @return The updated state to apply to InputControl - */ - const unitControlStateReducer: StateReducer = ( state, action ) => { - const nextState = { ...state }; - - /* - * On commits (when pressing ENTER and on blur if - * isPressEnterToChange is true), if a parse has been performed - * then use that result to update the state. - */ - if ( action.type === inputControlActionTypes.COMMIT ) { - if ( refParsedQuantity.current !== undefined ) { - nextState.value = ( - refParsedQuantity.current ?? '' - ).toString(); - refParsedQuantity.current = undefined; + let handleOnKeyDown; + if ( ! disableUnits && isUnitSelectTabbable && units.length ) { + handleOnKeyDown = ( event: KeyboardEvent< HTMLInputElement > ) => { + props.onKeyDown?.( event ); + // Does the key match the first character of any units? + if ( reFirstCharacterOfUnits.test( event.key ) ) { + // Moves focus to the UnitSelectControl. + refInputSuffix.current?.focus(); } - } - - return nextState; - }; - - let stateReducer: StateReducer = unitControlStateReducer; - if ( stateReducerProp ) { - stateReducer = ( state, action ) => { - const baseState = unitControlStateReducer( state, action ); - return stateReducerProp( baseState, action ); }; } + const refInputSuffix = useRef< HTMLSelectElement | null >( null ); const inputSuffix = ! disableUnits ? ( ) : null; @@ -262,7 +196,6 @@ function UnforwardedUnitControl( return ( ); diff --git a/packages/components/src/unit-control/unit-select-control.tsx b/packages/components/src/unit-control/unit-select-control.tsx index 844415fd0e426..99f481ae506fd 100644 --- a/packages/components/src/unit-control/unit-select-control.tsx +++ b/packages/components/src/unit-control/unit-select-control.tsx @@ -2,7 +2,12 @@ * External dependencies */ import classnames from 'classnames'; -import type { ChangeEvent } from 'react'; +import type { ChangeEvent, ForwardedRef } from 'react'; + +/** + * WordPress dependencies + */ +import { forwardRef } from '@wordpress/element'; /** * Internal dependencies @@ -12,15 +17,18 @@ import { UnitSelect, UnitLabel } from './styles/unit-control-styles'; import { CSS_UNITS, hasUnits } from './utils'; import type { UnitSelectControlProps } from './types'; -export default function UnitSelectControl( { - className, - isUnitSelectTabbable: isTabbable = true, - onChange, - size = 'default', - unit = 'px', - units = CSS_UNITS, - ...props -}: WordPressComponentProps< UnitSelectControlProps, 'select', false > ) { +function UnitSelectControl( + { + className, + isUnitSelectTabbable: isTabbable = true, + onChange, + size = 'default', + unit = 'px', + units = CSS_UNITS, + ...props + }: WordPressComponentProps< UnitSelectControlProps, 'select', false >, + ref: ForwardedRef< any > +) { if ( ! hasUnits( units ) || units?.length === 1 ) { return ( ); } +export default forwardRef( UnitSelectControl ); From d54b6fc574eb32f9f12c074258eb362b137877d9 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Tue, 8 Mar 2022 18:40:00 -0800 Subject: [PATCH 02/19] Change default of `isPressEnterToChange` in `BoxControl` --- packages/components/src/box-control/unit-control.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/components/src/box-control/unit-control.tsx b/packages/components/src/box-control/unit-control.tsx index ea724e9a82a19..4572f84632739 100644 --- a/packages/components/src/box-control/unit-control.tsx +++ b/packages/components/src/box-control/unit-control.tsx @@ -39,7 +39,6 @@ export default function BoxUnitControl( { isFirst={ isFirst } isLast={ isLast } isOnly={ isOnly } - isPressEnterToChange isResetValueOnUnitChange={ false } value={ value } { ...props } From befab108db9f09174b6440563d03379ad17bc0da Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Tue, 8 Mar 2022 18:42:14 -0800 Subject: [PATCH 03/19] Update tests --- .../components/src/box-control/test/index.tsx | 123 +++++--------- .../src/unit-control/test/index.tsx | 156 +++--------------- 2 files changed, 61 insertions(+), 218 deletions(-) diff --git a/packages/components/src/box-control/test/index.tsx b/packages/components/src/box-control/test/index.tsx index acd09b26ba8ab..dd134c17501fe 100644 --- a/packages/components/src/box-control/test/index.tsx +++ b/packages/components/src/box-control/test/index.tsx @@ -33,7 +33,7 @@ describe( 'BoxControl', () => { render( {} } /> ); expect( - screen.getByRole( 'textbox', { name: 'Box Control' } ) + screen.getByRole( 'spinbutton', { name: 'Box Control' } ) ).toBeVisible(); } ); @@ -42,19 +42,13 @@ describe( 'BoxControl', () => { render( {} } /> ); - const input = screen.getByRole( 'textbox', { + const input = screen.getByRole( 'spinbutton', { name: 'Box Control', } ); - await user.type( input, '100%' ); - await user.keyboard( '{Enter}' ); + await user.type( input, '100' ); - expect( input ).toHaveValue( '100' ); - expect( - screen.getByRole( 'combobox', { - name: 'Select unit', - } ) - ).toHaveValue( '%' ); + expect( input ).toHaveValue( 100 ); } ); } ); @@ -64,23 +58,17 @@ describe( 'BoxControl', () => { render( {} } /> ); - const input = screen.getByRole( 'textbox', { + const input = screen.getByRole( 'spinbutton', { name: 'Box Control', } ); - const select = screen.getByRole( 'combobox', { - name: 'Select unit', - } ); - await user.type( input, '100px' ); - await user.keyboard( '{Enter}' ); + await user.type( input, '100' ); - expect( input ).toHaveValue( '100' ); - expect( select ).toHaveValue( 'px' ); + expect( input ).toHaveValue( 100 ); await user.click( screen.getByRole( 'button', { name: 'Reset' } ) ); - expect( input ).toHaveValue( '' ); - expect( select ).toHaveValue( 'px' ); + expect( input ).toHaveValue( null ); } ); it( 'should reset values when clicking Reset, if controlled', async () => { @@ -88,23 +76,17 @@ describe( 'BoxControl', () => { render( ); - const input = screen.getByRole( 'textbox', { + const input = screen.getByRole( 'spinbutton', { name: 'Box Control', } ); - const select = screen.getByRole( 'combobox', { - name: 'Select unit', - } ); - await user.type( input, '100px' ); - await user.keyboard( '{Enter}' ); + await user.type( input, '100' ); - expect( input ).toHaveValue( '100' ); - expect( select ).toHaveValue( 'px' ); + expect( input ).toHaveValue( 100 ); await user.click( screen.getByRole( 'button', { name: 'Reset' } ) ); - expect( input ).toHaveValue( '' ); - expect( select ).toHaveValue( 'px' ); + expect( input ).toHaveValue( null ); } ); it( 'should reset values when clicking Reset, if controlled <-> uncontrolled state changes', async () => { @@ -112,23 +94,17 @@ describe( 'BoxControl', () => { render( ); - const input = screen.getByRole( 'textbox', { + const input = screen.getByRole( 'spinbutton', { name: 'Box Control', } ); - const select = screen.getByRole( 'combobox', { - name: 'Select unit', - } ); - await user.type( input, '100px' ); - await user.keyboard( '{Enter}' ); + await user.type( input, '100' ); - expect( input ).toHaveValue( '100' ); - expect( select ).toHaveValue( 'px' ); + expect( input ).toHaveValue( 100 ); await user.click( screen.getByRole( 'button', { name: 'Reset' } ) ); - expect( input ).toHaveValue( '' ); - expect( select ).toHaveValue( 'px' ); + expect( input ).toHaveValue( null ); } ); it( 'should persist cleared value when focus changes', async () => { @@ -137,26 +113,20 @@ describe( 'BoxControl', () => { render( spyChange( v ) } /> ); - const input = screen.getByRole( 'textbox', { + const input = screen.getByRole( 'spinbutton', { name: 'Box Control', } ); - await user.type( input, '100%' ); - await user.keyboard( '{Enter}' ); + await user.type( input, '100' ); - expect( input ).toHaveValue( '100' ); - expect( - screen.getByRole( 'combobox', { - name: 'Select unit', - } ) - ).toHaveValue( '%' ); + expect( input ).toHaveValue( 100 ); await user.clear( input ); - expect( input ).toHaveValue( '' ); + expect( input ).toHaveValue( null ); // Clicking document.body to trigger a blur event on the input. await user.click( document.body ); - expect( input ).toHaveValue( '' ); + expect( input ).toHaveValue( null ); expect( spyChange ).toHaveBeenLastCalledWith( { top: undefined, right: undefined, @@ -177,29 +147,22 @@ describe( 'BoxControl', () => { ); await user.type( - screen.getByRole( 'textbox', { name: 'Top' } ), - '100px' + screen.getByRole( 'spinbutton', { name: 'Top' } ), + '100' ); - await user.keyboard( '{Enter}' ); expect( - screen.getByRole( 'textbox', { name: 'Top' } ) - ).toHaveValue( '100' ); + screen.getByRole( 'spinbutton', { name: 'Top' } ) + ).toHaveValue( 100 ); expect( - screen.getByRole( 'textbox', { name: 'Right' } ) + screen.getByRole( 'spinbutton', { name: 'Right' } ) ).not.toHaveValue(); expect( - screen.getByRole( 'textbox', { name: 'Bottom' } ) + screen.getByRole( 'spinbutton', { name: 'Bottom' } ) ).not.toHaveValue(); expect( - screen.getByRole( 'textbox', { name: 'Left' } ) + screen.getByRole( 'spinbutton', { name: 'Left' } ) ).not.toHaveValue(); - - screen - .getAllByRole( 'combobox', { name: 'Select unit' } ) - .forEach( ( combobox ) => { - expect( combobox ).toHaveValue( 'px' ); - } ); } ); it( 'should update a whole axis when value is changed when unlinked', async () => { @@ -212,25 +175,18 @@ describe( 'BoxControl', () => { ); await user.type( - screen.getByRole( 'textbox', { + screen.getByRole( 'spinbutton', { name: 'Vertical', } ), - '100px' + '100' ); - await user.keyboard( '{Enter}' ); expect( - screen.getByRole( 'textbox', { name: 'Vertical' } ) - ).toHaveValue( '100' ); + screen.getByRole( 'spinbutton', { name: 'Vertical' } ) + ).toHaveValue( 100 ); expect( - screen.getByRole( 'textbox', { name: 'Horizontal' } ) + screen.getByRole( 'spinbutton', { name: 'Horizontal' } ) ).not.toHaveValue(); - - screen - .getAllByRole( 'combobox', { name: 'Select unit' } ) - .forEach( ( combobox ) => { - expect( combobox ).toHaveValue( 'px' ); - } ); } ); } ); @@ -326,18 +282,17 @@ describe( 'BoxControl', () => { render( ); await user.type( - screen.getByRole( 'textbox', { + screen.getByRole( 'spinbutton', { name: 'Box Control', } ), - '7.5rem' + '7.5' ); - await user.keyboard( '{Enter}' ); expect( setState ).toHaveBeenCalledWith( { - top: '7.5rem', - right: '7.5rem', - bottom: '7.5rem', - left: '7.5rem', + top: '7.5px', + right: '7.5px', + bottom: '7.5px', + left: '7.5px', } ); } ); diff --git a/packages/components/src/unit-control/test/index.tsx b/packages/components/src/unit-control/test/index.tsx index b4103ac4b2b8b..7b713396231e2 100644 --- a/packages/components/src/unit-control/test/index.tsx +++ b/packages/components/src/unit-control/test/index.tsx @@ -233,9 +233,9 @@ describe( 'UnitControl', () => { // Input type is `text` when the `isPressEnterToChange` prop is passed const input = getInput( { isInputTypeText: true } ); await user.clear( input ); - await user.type( input, '300px' ); + await user.type( input, '300' ); - expect( input.value ).toBe( '300px' ); + expect( input.value ).toBe( '300' ); expect( state ).toBe( 50 ); await user.keyboard( '{Escape}' ); @@ -277,10 +277,9 @@ describe( 'UnitControl', () => { expect( onBlurSpy ).toHaveBeenCalledTimes( 1 ); } ); - it( 'should invoke onChange and onUnitChange callbacks when isPressEnterToChange is true and the component is blurred with an uncommitted value', async () => { + it( 'should invoke onChange when isPressEnterToChange is true and the input is blurred with an uncommitted value', async () => { const user = userEvent.setup(); - const onUnitChangeSpy = jest.fn(); const onChangeSpy = jest.fn(); let state: string | undefined = '15px'; @@ -293,7 +292,6 @@ describe( 'UnitControl', () => { ); @@ -301,23 +299,15 @@ describe( 'UnitControl', () => { // Input type is `text` when the `isPressEnterToChange` prop is passed const input = getInput( { isInputTypeText: true } ); await user.clear( input ); - await user.type( input, '41vh' ); - - // This is because `isPressEnterToChange` is `true` - expect( onChangeSpy ).not.toHaveBeenCalled(); - expect( onUnitChangeSpy ).not.toHaveBeenCalled(); - - // Clicking document.body to trigger a blur event on the input. - await user.click( document.body ); + // Typing the first letter of a unit blurs the input. + await user.type( input, '41v' ); + // Called only once because `isPressEnterToChange` is `true`. expect( onChangeSpy ).toHaveBeenCalledTimes( 1 ); - expect( onChangeSpy ).toHaveBeenLastCalledWith( '41vh' ); - expect( onUnitChangeSpy ).toHaveBeenCalledTimes( 1 ); - expect( onUnitChangeSpy ).toHaveBeenLastCalledWith( - 'vh', - expect.anything() - ); + // True for the test environment but in common browsers this would + // be last called with '41vw' (after the call with '41px'). + expect( onChangeSpy ).toHaveBeenLastCalledWith( '41px' ); } ); it( 'should update value correctly when typed and blurred when a single unit is passed', async () => { @@ -560,121 +550,6 @@ describe( 'UnitControl', () => { } ); describe( 'Unit Parser', () => { - it( 'should parse unit from input', async () => { - const user = userEvent.setup(); - - let state = '10px'; - const setState = jest.fn( ( nextState ) => ( state = nextState ) ); - - render( - - ); - - // Input type is `text` when the `isPressEnterToChange` prop is passed - const input = getInput( { isInputTypeText: true } ); - await user.clear( input ); - await user.type( input, '55 em' ); - await user.keyboard( '{Enter}' ); - - expect( state ).toBe( '55em' ); - } ); - - it( 'should parse PX unit from input', async () => { - const user = userEvent.setup(); - - let state = '10px'; - const setState = jest.fn( ( nextState ) => ( state = nextState ) ); - - render( - - ); - - // Input type is `text` when the `isPressEnterToChange` prop is passed - const input = getInput( { isInputTypeText: true } ); - await user.clear( input ); - await user.type( input, '61 PX' ); - await user.keyboard( '{Enter}' ); - - expect( state ).toBe( '61px' ); - } ); - - it( 'should parse EM unit from input', async () => { - const user = userEvent.setup(); - - let state = '10px'; - const setState = jest.fn( ( nextState ) => ( state = nextState ) ); - - render( - - ); - - // Input type is `text` when the `isPressEnterToChange` prop is passed - const input = getInput( { isInputTypeText: true } ); - await user.clear( input ); - await user.type( input, '55 em' ); - await user.keyboard( '{Enter}' ); - - expect( state ).toBe( '55em' ); - } ); - - it( 'should parse % unit from input', async () => { - const user = userEvent.setup(); - - let state = '10px'; - const setState = jest.fn( ( nextState ) => ( state = nextState ) ); - - render( - - ); - - // Input type is `text` when the `isPressEnterToChange` prop is passed - const input = getInput( { isInputTypeText: true } ); - await user.clear( input ); - await user.type( input, '-10 %' ); - await user.keyboard( '{Enter}' ); - - expect( state ).toBe( '-10%' ); - } ); - - it( 'should parse REM unit from input', async () => { - const user = userEvent.setup(); - - let state = '10px'; - const setState = jest.fn( ( nextState ) => ( state = nextState ) ); - - render( - - ); - - // Input type is `text` when the `isPressEnterToChange` prop is passed - const input = getInput( { isInputTypeText: true } ); - await user.clear( input ); - await user.type( input, '123 rEm ' ); - await user.keyboard( '{Enter}' ); - - expect( state ).toBe( '123rem' ); - } ); - it( 'should update unit after initial render and with new unit prop', async () => { const { rerender } = render( ); @@ -711,4 +586,17 @@ describe( 'UnitControl', () => { expect( options.length ).toBe( 3 ); } ); } ); + + describe( 'Unit switching convenience', () => { + it( 'should focus unit select when a charater matches the first of one of the units', async () => { + const user = userEvent.setup(); + render( ); + + const input = getInput(); + await user.clear( input ); + await user.type( input, '55 e' ); + + expect( getSelect() ).toHaveFocus(); + } ); + } ); } ); From 1cfad18c19867d5e3c43bc76ee2f0d0d5341edb0 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Tue, 8 Mar 2022 22:07:23 -0800 Subject: [PATCH 04/19] Avoid potential undefined --- packages/components/src/unit-control/index.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/components/src/unit-control/index.tsx b/packages/components/src/unit-control/index.tsx index c8dd72ba45c0a..4c2ba0aa3a2f1 100644 --- a/packages/components/src/unit-control/index.tsx +++ b/packages/components/src/unit-control/index.tsx @@ -77,7 +77,7 @@ function UnforwardedUnitControl( const firstCharacters = list.reduce( ( carry, { value } ) => { const first = value.substr( 0, 1 ); return carry.includes( first ) ? carry : `${ carry }|${ first }`; - }, list[ 0 ].value.substr( 0, 1 ) ); + }, list[ 0 ]?.value.substr( 0, 1 ) ); return [ list, new RegExp( `^(?:${ firstCharacters })$` ) ]; }, [ nonNullValueProp, unitProp, unitsProp ] ); const [ parsedQuantity, parsedUnit ] = getParsedQuantityAndUnit( From 213edc056086b94ace6997608dd48345e81a8c63 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Thu, 24 Mar 2022 13:05:43 -0700 Subject: [PATCH 05/19] Remove extraneous null typing Co-authored-by: Marco Ciampini --- packages/components/src/unit-control/index.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/components/src/unit-control/index.tsx b/packages/components/src/unit-control/index.tsx index 4c2ba0aa3a2f1..138b56bdd83db 100644 --- a/packages/components/src/unit-control/index.tsx +++ b/packages/components/src/unit-control/index.tsx @@ -167,7 +167,7 @@ function UnforwardedUnitControl( }; } - const refInputSuffix = useRef< HTMLSelectElement | null >( null ); + const refInputSuffix = useRef< HTMLSelectElement >( null ); const inputSuffix = ! disableUnits ? ( Date: Thu, 24 Mar 2022 13:07:55 -0700 Subject: [PATCH 06/19] Make key matching regex case-insensitive Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> --- packages/components/src/unit-control/index.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/components/src/unit-control/index.tsx b/packages/components/src/unit-control/index.tsx index 138b56bdd83db..ee9d38e3d1c78 100644 --- a/packages/components/src/unit-control/index.tsx +++ b/packages/components/src/unit-control/index.tsx @@ -78,7 +78,7 @@ function UnforwardedUnitControl( const first = value.substr( 0, 1 ); return carry.includes( first ) ? carry : `${ carry }|${ first }`; }, list[ 0 ]?.value.substr( 0, 1 ) ); - return [ list, new RegExp( `^(?:${ firstCharacters })$` ) ]; + return [ list, new RegExp( `^(?:${ firstCharacters })$`, 'i' ) ]; }, [ nonNullValueProp, unitProp, unitsProp ] ); const [ parsedQuantity, parsedUnit ] = getParsedQuantityAndUnit( nonNullValueProp, From b09c7b23675dab762afad436ed4369a63d383e2a Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Thu, 24 Mar 2022 13:38:23 -0700 Subject: [PATCH 07/19] Revert removed condtional type="text" on `UnitControl` --- packages/components/src/unit-control/index.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/components/src/unit-control/index.tsx b/packages/components/src/unit-control/index.tsx index ee9d38e3d1c78..588eba5b8c5d4 100644 --- a/packages/components/src/unit-control/index.tsx +++ b/packages/components/src/unit-control/index.tsx @@ -208,6 +208,7 @@ function UnforwardedUnitControl( ref={ forwardedRef } size={ size } suffix={ inputSuffix } + type={ isPressEnterToChange ? 'text' : 'number' } value={ parsedQuantity ?? '' } step={ step } onFocus={ onFocusProp } From a7075b8ea1b0f1feb8f5102c61f53c9dc3cd7dfd Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Thu, 24 Mar 2022 13:44:46 -0700 Subject: [PATCH 08/19] Forward __unstableStateReducer through `UnitControl` --- packages/components/src/unit-control/index.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/components/src/unit-control/index.tsx b/packages/components/src/unit-control/index.tsx index 588eba5b8c5d4..5e6ad38ff0713 100644 --- a/packages/components/src/unit-control/index.tsx +++ b/packages/components/src/unit-control/index.tsx @@ -35,7 +35,7 @@ function UnforwardedUnitControl( forwardedRef: ForwardedRef< any > ) { const { - __unstableStateReducer: stateReducerProp, + __unstableStateReducer, autoComplete = 'off', // @ts-expect-error Ensure that children is omitted from restProps children, @@ -212,6 +212,7 @@ function UnforwardedUnitControl( value={ parsedQuantity ?? '' } step={ step } onFocus={ onFocusProp } + __unstableStateReducer={ __unstableStateReducer } /> ); } From 8d761b57e1cd15fd086fb478f9c2904658d0fc93 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Wed, 6 Apr 2022 18:20:23 -0700 Subject: [PATCH 09/19] Avoid intefering with paste or other shortcuts --- packages/components/src/unit-control/index.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/components/src/unit-control/index.tsx b/packages/components/src/unit-control/index.tsx index 5e6ad38ff0713..789177ac5ace6 100644 --- a/packages/components/src/unit-control/index.tsx +++ b/packages/components/src/unit-control/index.tsx @@ -159,6 +159,9 @@ function UnforwardedUnitControl( if ( ! disableUnits && isUnitSelectTabbable && units.length ) { handleOnKeyDown = ( event: KeyboardEvent< HTMLInputElement > ) => { props.onKeyDown?.( event ); + // Bails if the meta key is pressed to not interfere with shortcuts, + // the prime example being pastes. + if ( event.metaKey ) return; // Does the key match the first character of any units? if ( reFirstCharacterOfUnits.test( event.key ) ) { // Moves focus to the UnitSelectControl. From 0877a4633be76ed308a9e231f5c8c31fd8f9bf10 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Mon, 5 Jun 2023 22:36:03 -0700 Subject: [PATCH 10/19] Revert `BoxControl` changes --- .../components/src/box-control/test/index.tsx | 123 ++++++++++++------ .../src/box-control/unit-control.tsx | 1 + 2 files changed, 85 insertions(+), 39 deletions(-) diff --git a/packages/components/src/box-control/test/index.tsx b/packages/components/src/box-control/test/index.tsx index dd134c17501fe..acd09b26ba8ab 100644 --- a/packages/components/src/box-control/test/index.tsx +++ b/packages/components/src/box-control/test/index.tsx @@ -33,7 +33,7 @@ describe( 'BoxControl', () => { render( {} } /> ); expect( - screen.getByRole( 'spinbutton', { name: 'Box Control' } ) + screen.getByRole( 'textbox', { name: 'Box Control' } ) ).toBeVisible(); } ); @@ -42,13 +42,19 @@ describe( 'BoxControl', () => { render( {} } /> ); - const input = screen.getByRole( 'spinbutton', { + const input = screen.getByRole( 'textbox', { name: 'Box Control', } ); - await user.type( input, '100' ); + await user.type( input, '100%' ); + await user.keyboard( '{Enter}' ); - expect( input ).toHaveValue( 100 ); + expect( input ).toHaveValue( '100' ); + expect( + screen.getByRole( 'combobox', { + name: 'Select unit', + } ) + ).toHaveValue( '%' ); } ); } ); @@ -58,17 +64,23 @@ describe( 'BoxControl', () => { render( {} } /> ); - const input = screen.getByRole( 'spinbutton', { + const input = screen.getByRole( 'textbox', { name: 'Box Control', } ); + const select = screen.getByRole( 'combobox', { + name: 'Select unit', + } ); - await user.type( input, '100' ); + await user.type( input, '100px' ); + await user.keyboard( '{Enter}' ); - expect( input ).toHaveValue( 100 ); + expect( input ).toHaveValue( '100' ); + expect( select ).toHaveValue( 'px' ); await user.click( screen.getByRole( 'button', { name: 'Reset' } ) ); - expect( input ).toHaveValue( null ); + expect( input ).toHaveValue( '' ); + expect( select ).toHaveValue( 'px' ); } ); it( 'should reset values when clicking Reset, if controlled', async () => { @@ -76,17 +88,23 @@ describe( 'BoxControl', () => { render( ); - const input = screen.getByRole( 'spinbutton', { + const input = screen.getByRole( 'textbox', { name: 'Box Control', } ); + const select = screen.getByRole( 'combobox', { + name: 'Select unit', + } ); - await user.type( input, '100' ); + await user.type( input, '100px' ); + await user.keyboard( '{Enter}' ); - expect( input ).toHaveValue( 100 ); + expect( input ).toHaveValue( '100' ); + expect( select ).toHaveValue( 'px' ); await user.click( screen.getByRole( 'button', { name: 'Reset' } ) ); - expect( input ).toHaveValue( null ); + expect( input ).toHaveValue( '' ); + expect( select ).toHaveValue( 'px' ); } ); it( 'should reset values when clicking Reset, if controlled <-> uncontrolled state changes', async () => { @@ -94,17 +112,23 @@ describe( 'BoxControl', () => { render( ); - const input = screen.getByRole( 'spinbutton', { + const input = screen.getByRole( 'textbox', { name: 'Box Control', } ); + const select = screen.getByRole( 'combobox', { + name: 'Select unit', + } ); - await user.type( input, '100' ); + await user.type( input, '100px' ); + await user.keyboard( '{Enter}' ); - expect( input ).toHaveValue( 100 ); + expect( input ).toHaveValue( '100' ); + expect( select ).toHaveValue( 'px' ); await user.click( screen.getByRole( 'button', { name: 'Reset' } ) ); - expect( input ).toHaveValue( null ); + expect( input ).toHaveValue( '' ); + expect( select ).toHaveValue( 'px' ); } ); it( 'should persist cleared value when focus changes', async () => { @@ -113,20 +137,26 @@ describe( 'BoxControl', () => { render( spyChange( v ) } /> ); - const input = screen.getByRole( 'spinbutton', { + const input = screen.getByRole( 'textbox', { name: 'Box Control', } ); - await user.type( input, '100' ); + await user.type( input, '100%' ); + await user.keyboard( '{Enter}' ); - expect( input ).toHaveValue( 100 ); + expect( input ).toHaveValue( '100' ); + expect( + screen.getByRole( 'combobox', { + name: 'Select unit', + } ) + ).toHaveValue( '%' ); await user.clear( input ); - expect( input ).toHaveValue( null ); + expect( input ).toHaveValue( '' ); // Clicking document.body to trigger a blur event on the input. await user.click( document.body ); - expect( input ).toHaveValue( null ); + expect( input ).toHaveValue( '' ); expect( spyChange ).toHaveBeenLastCalledWith( { top: undefined, right: undefined, @@ -147,22 +177,29 @@ describe( 'BoxControl', () => { ); await user.type( - screen.getByRole( 'spinbutton', { name: 'Top' } ), - '100' + screen.getByRole( 'textbox', { name: 'Top' } ), + '100px' ); + await user.keyboard( '{Enter}' ); expect( - screen.getByRole( 'spinbutton', { name: 'Top' } ) - ).toHaveValue( 100 ); + screen.getByRole( 'textbox', { name: 'Top' } ) + ).toHaveValue( '100' ); expect( - screen.getByRole( 'spinbutton', { name: 'Right' } ) + screen.getByRole( 'textbox', { name: 'Right' } ) ).not.toHaveValue(); expect( - screen.getByRole( 'spinbutton', { name: 'Bottom' } ) + screen.getByRole( 'textbox', { name: 'Bottom' } ) ).not.toHaveValue(); expect( - screen.getByRole( 'spinbutton', { name: 'Left' } ) + screen.getByRole( 'textbox', { name: 'Left' } ) ).not.toHaveValue(); + + screen + .getAllByRole( 'combobox', { name: 'Select unit' } ) + .forEach( ( combobox ) => { + expect( combobox ).toHaveValue( 'px' ); + } ); } ); it( 'should update a whole axis when value is changed when unlinked', async () => { @@ -175,18 +212,25 @@ describe( 'BoxControl', () => { ); await user.type( - screen.getByRole( 'spinbutton', { + screen.getByRole( 'textbox', { name: 'Vertical', } ), - '100' + '100px' ); + await user.keyboard( '{Enter}' ); expect( - screen.getByRole( 'spinbutton', { name: 'Vertical' } ) - ).toHaveValue( 100 ); + screen.getByRole( 'textbox', { name: 'Vertical' } ) + ).toHaveValue( '100' ); expect( - screen.getByRole( 'spinbutton', { name: 'Horizontal' } ) + screen.getByRole( 'textbox', { name: 'Horizontal' } ) ).not.toHaveValue(); + + screen + .getAllByRole( 'combobox', { name: 'Select unit' } ) + .forEach( ( combobox ) => { + expect( combobox ).toHaveValue( 'px' ); + } ); } ); } ); @@ -282,17 +326,18 @@ describe( 'BoxControl', () => { render( ); await user.type( - screen.getByRole( 'spinbutton', { + screen.getByRole( 'textbox', { name: 'Box Control', } ), - '7.5' + '7.5rem' ); + await user.keyboard( '{Enter}' ); expect( setState ).toHaveBeenCalledWith( { - top: '7.5px', - right: '7.5px', - bottom: '7.5px', - left: '7.5px', + top: '7.5rem', + right: '7.5rem', + bottom: '7.5rem', + left: '7.5rem', } ); } ); diff --git a/packages/components/src/box-control/unit-control.tsx b/packages/components/src/box-control/unit-control.tsx index 4572f84632739..ea724e9a82a19 100644 --- a/packages/components/src/box-control/unit-control.tsx +++ b/packages/components/src/box-control/unit-control.tsx @@ -39,6 +39,7 @@ export default function BoxUnitControl( { isFirst={ isFirst } isLast={ isLast } isOnly={ isOnly } + isPressEnterToChange isResetValueOnUnitChange={ false } value={ value } { ...props } From 8e64f42067ae1bb57112280cda30cc581c03769c Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Tue, 6 Jun 2023 08:49:36 -0700 Subject: [PATCH 11/19] Update unit tests for `BoxControl` --- .../components/src/box-control/test/index.tsx | 69 +++++-------------- 1 file changed, 16 insertions(+), 53 deletions(-) diff --git a/packages/components/src/box-control/test/index.tsx b/packages/components/src/box-control/test/index.tsx index acd09b26ba8ab..eec363440d7a3 100644 --- a/packages/components/src/box-control/test/index.tsx +++ b/packages/components/src/box-control/test/index.tsx @@ -46,15 +46,10 @@ describe( 'BoxControl', () => { name: 'Box Control', } ); - await user.type( input, '100%' ); + await user.type( input, '100' ); await user.keyboard( '{Enter}' ); expect( input ).toHaveValue( '100' ); - expect( - screen.getByRole( 'combobox', { - name: 'Select unit', - } ) - ).toHaveValue( '%' ); } ); } ); @@ -67,20 +62,15 @@ describe( 'BoxControl', () => { const input = screen.getByRole( 'textbox', { name: 'Box Control', } ); - const select = screen.getByRole( 'combobox', { - name: 'Select unit', - } ); - await user.type( input, '100px' ); + await user.type( input, '100' ); await user.keyboard( '{Enter}' ); expect( input ).toHaveValue( '100' ); - expect( select ).toHaveValue( 'px' ); await user.click( screen.getByRole( 'button', { name: 'Reset' } ) ); expect( input ).toHaveValue( '' ); - expect( select ).toHaveValue( 'px' ); } ); it( 'should reset values when clicking Reset, if controlled', async () => { @@ -91,20 +81,15 @@ describe( 'BoxControl', () => { const input = screen.getByRole( 'textbox', { name: 'Box Control', } ); - const select = screen.getByRole( 'combobox', { - name: 'Select unit', - } ); - await user.type( input, '100px' ); + await user.type( input, '100' ); await user.keyboard( '{Enter}' ); expect( input ).toHaveValue( '100' ); - expect( select ).toHaveValue( 'px' ); await user.click( screen.getByRole( 'button', { name: 'Reset' } ) ); expect( input ).toHaveValue( '' ); - expect( select ).toHaveValue( 'px' ); } ); it( 'should reset values when clicking Reset, if controlled <-> uncontrolled state changes', async () => { @@ -115,20 +100,15 @@ describe( 'BoxControl', () => { const input = screen.getByRole( 'textbox', { name: 'Box Control', } ); - const select = screen.getByRole( 'combobox', { - name: 'Select unit', - } ); - await user.type( input, '100px' ); + await user.type( input, '100' ); await user.keyboard( '{Enter}' ); expect( input ).toHaveValue( '100' ); - expect( select ).toHaveValue( 'px' ); await user.click( screen.getByRole( 'button', { name: 'Reset' } ) ); expect( input ).toHaveValue( '' ); - expect( select ).toHaveValue( 'px' ); } ); it( 'should persist cleared value when focus changes', async () => { @@ -141,15 +121,10 @@ describe( 'BoxControl', () => { name: 'Box Control', } ); - await user.type( input, '100%' ); + await user.type( input, '100' ); await user.keyboard( '{Enter}' ); expect( input ).toHaveValue( '100' ); - expect( - screen.getByRole( 'combobox', { - name: 'Select unit', - } ) - ).toHaveValue( '%' ); await user.clear( input ); expect( input ).toHaveValue( '' ); @@ -178,9 +153,8 @@ describe( 'BoxControl', () => { await user.type( screen.getByRole( 'textbox', { name: 'Top' } ), - '100px' + '100' ); - await user.keyboard( '{Enter}' ); expect( screen.getByRole( 'textbox', { name: 'Top' } ) @@ -194,12 +168,6 @@ describe( 'BoxControl', () => { expect( screen.getByRole( 'textbox', { name: 'Left' } ) ).not.toHaveValue(); - - screen - .getAllByRole( 'combobox', { name: 'Select unit' } ) - .forEach( ( combobox ) => { - expect( combobox ).toHaveValue( 'px' ); - } ); } ); it( 'should update a whole axis when value is changed when unlinked', async () => { @@ -215,9 +183,8 @@ describe( 'BoxControl', () => { screen.getByRole( 'textbox', { name: 'Vertical', } ), - '100px' + '100' ); - await user.keyboard( '{Enter}' ); expect( screen.getByRole( 'textbox', { name: 'Vertical' } ) @@ -225,12 +192,6 @@ describe( 'BoxControl', () => { expect( screen.getByRole( 'textbox', { name: 'Horizontal' } ) ).not.toHaveValue(); - - screen - .getAllByRole( 'combobox', { name: 'Select unit' } ) - .forEach( ( combobox ) => { - expect( combobox ).toHaveValue( 'px' ); - } ); } ); } ); @@ -325,19 +286,21 @@ describe( 'BoxControl', () => { render( ); + // Typing the first letter of a unit blurs the input. await user.type( screen.getByRole( 'textbox', { name: 'Box Control', } ), - '7.5rem' + '7r' ); - await user.keyboard( '{Enter}' ); - expect( setState ).toHaveBeenCalledWith( { - top: '7.5rem', - right: '7.5rem', - bottom: '7.5rem', - left: '7.5rem', + // Due to the test environment not being a browser the units here + // are still "px". In a browser they would be expected to be "rem". + expect( setState ).toHaveBeenLastCalledWith( { + top: '7px', + right: '7px', + bottom: '7px', + left: '7px', } ); } ); From 624ed26008441d8a7c86d848e93af0e60cba49f9 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Mon, 12 Jun 2023 17:06:55 -0700 Subject: [PATCH 12/19] Optimize and guard creation of regex Reduce starting from the second unit and cover null safety --- packages/components/src/unit-control/index.tsx | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/components/src/unit-control/index.tsx b/packages/components/src/unit-control/index.tsx index 789177ac5ace6..03f464d178764 100644 --- a/packages/components/src/unit-control/index.tsx +++ b/packages/components/src/unit-control/index.tsx @@ -74,10 +74,11 @@ function UnforwardedUnitControl( unitProp, unitsProp ); - const firstCharacters = list.reduce( ( carry, { value } ) => { - const first = value.substr( 0, 1 ); + const [ { value: firstUnitValue = '' } = {}, ...rest ] = list; + const firstCharacters = rest.reduce( ( carry, { value } ) => { + const first = value?.substring( 0, 1 ) || ''; return carry.includes( first ) ? carry : `${ carry }|${ first }`; - }, list[ 0 ]?.value.substr( 0, 1 ) ); + }, firstUnitValue.substring( 0, 1 ) ); return [ list, new RegExp( `^(?:${ firstCharacters })$`, 'i' ) ]; }, [ nonNullValueProp, unitProp, unitsProp ] ); const [ parsedQuantity, parsedUnit ] = getParsedQuantityAndUnit( From 6f1d706518f067c2d9582b8429ab308927bb8ede Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Mon, 12 Jun 2023 17:13:30 -0700 Subject: [PATCH 13/19] Revise style --- packages/components/src/unit-control/index.tsx | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/packages/components/src/unit-control/index.tsx b/packages/components/src/unit-control/index.tsx index 03f464d178764..847056ae4da47 100644 --- a/packages/components/src/unit-control/index.tsx +++ b/packages/components/src/unit-control/index.tsx @@ -160,14 +160,11 @@ function UnforwardedUnitControl( if ( ! disableUnits && isUnitSelectTabbable && units.length ) { handleOnKeyDown = ( event: KeyboardEvent< HTMLInputElement > ) => { props.onKeyDown?.( event ); - // Bails if the meta key is pressed to not interfere with shortcuts, - // the prime example being pastes. - if ( event.metaKey ) return; - // Does the key match the first character of any units? - if ( reFirstCharacterOfUnits.test( event.key ) ) { - // Moves focus to the UnitSelectControl. + // Unless the meta key was pressed (to avoid interfering with + // shortcuts, e.g. pastes), moves focus to the unit select if a key + // matches the first character of a unit. + if ( ! event.metaKey && reFirstCharacterOfUnits.test( event.key ) ) refInputSuffix.current?.focus(); - } }; } From c205c7d6a8c0583bf2cbf1550748084fa0375db6 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Mon, 12 Jun 2023 17:19:17 -0700 Subject: [PATCH 14/19] Make test of unit switching convenience run for each css unit --- .../src/unit-control/test/index.tsx | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/packages/components/src/unit-control/test/index.tsx b/packages/components/src/unit-control/test/index.tsx index 7b713396231e2..1677ab371e3f3 100644 --- a/packages/components/src/unit-control/test/index.tsx +++ b/packages/components/src/unit-control/test/index.tsx @@ -13,7 +13,7 @@ import { useState } from '@wordpress/element'; * Internal dependencies */ import UnitControl from '..'; -import { parseQuantityAndUnitFromRawValue } from '../utils'; +import { CSS_UNITS, parseQuantityAndUnitFromRawValue } from '../utils'; import type { UnitControlOnChangeCallback } from '../types'; const getInput = ( { @@ -588,15 +588,20 @@ describe( 'UnitControl', () => { } ); describe( 'Unit switching convenience', () => { - it( 'should focus unit select when a charater matches the first of one of the units', async () => { - const user = userEvent.setup(); - render( ); - - const input = getInput(); - await user.clear( input ); - await user.type( input, '55 e' ); - - expect( getSelect() ).toHaveFocus(); - } ); + it.each( CSS_UNITS.map( ( { value } ) => value ) )( + 'should move focus from the input to the unit select when typing the first character of %p', + async ( testUnit ) => { + const user = userEvent.setup(); + render( ); + + const input = getInput(); + await user.clear( input ); + await user.type( input, `55${ testUnit }` ); + + expect( getSelect() ).toHaveFocus(); + // The unit character was not entered in the input. + expect( input ).toHaveValue( 55 ); + } + ); } ); } ); From fcff0b13e686d45bc89727a2dba817a1779d57a6 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Mon, 12 Jun 2023 17:34:54 -0700 Subject: [PATCH 15/19] Add changelog entry --- packages/components/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index e370d4450261e..b7d711951caed 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -17,6 +17,10 @@ - `ClipboardButton`: Convert to TypeScript ([#51334](https://github.com/WordPress/gutenberg/pull/51334)). - `Toolbar`: Replace `reakit` dependency with `@ariakit/react` ([#51623](https://github.com/WordPress/gutenberg/pull/51623)). +### Enhancements + +- `UnitControl`: Revamp support for changing unit by typing ([#39303](https://github.com/WordPress/gutenberg/pull/39303)). + ## 25.1.0 (2023-06-07) ### Enhancements From 1b386cffcd5413b4612e58fc81ecb29142465bbc Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Wed, 21 Jun 2023 15:02:05 +0200 Subject: [PATCH 16/19] Remove custom `onChange` callback on Storybook, as it interferes with the actions tab --- packages/components/src/unit-control/stories/index.tsx | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/components/src/unit-control/stories/index.tsx b/packages/components/src/unit-control/stories/index.tsx index 6fe4b49a5687d..9721d2de631a4 100644 --- a/packages/components/src/unit-control/stories/index.tsx +++ b/packages/components/src/unit-control/stories/index.tsx @@ -69,10 +69,6 @@ export const PressEnterToChange: ComponentStory< typeof UnitControl > = PressEnterToChange.args = { ...Default.args, isPressEnterToChange: true, - onChange: ( v ) => { - // eslint-disable-next-line no-console - console.log( v ); - }, }; /** From 92bbdaa0762ae034511786ff578857d9b25f2856 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Wed, 21 Jun 2023 15:03:53 +0200 Subject: [PATCH 17/19] Improve tests by avoiding local `state` variables and using onChange spies instead --- .../components/src/box-control/test/index.tsx | 24 +- .../src/unit-control/test/index.tsx | 213 ++++++++++-------- 2 files changed, 135 insertions(+), 102 deletions(-) diff --git a/packages/components/src/box-control/test/index.tsx b/packages/components/src/box-control/test/index.tsx index eec363440d7a3..b99c6264bbc56 100644 --- a/packages/components/src/box-control/test/index.tsx +++ b/packages/components/src/box-control/test/index.tsx @@ -282,21 +282,25 @@ describe( 'BoxControl', () => { describe( 'onChange updates', () => { it( 'should call onChange when values contain more than just CSS units', async () => { const user = userEvent.setup(); - const setState = jest.fn(); + const onChangeSpy = jest.fn(); - render( ); + render( ); - // Typing the first letter of a unit blurs the input. - await user.type( - screen.getByRole( 'textbox', { - name: 'Box Control', - } ), - '7r' - ); + const valueInput = screen.getByRole( 'textbox', { + name: 'Box Control', + } ); + const unitSelect = screen.getByRole( 'combobox', { + name: 'Select unit', + } ); + + // Typing the first letter of a unit blurs the input and focuses the combobox. + await user.type( valueInput, '7r' ); + + expect( unitSelect ).toHaveFocus(); // Due to the test environment not being a browser the units here // are still "px". In a browser they would be expected to be "rem". - expect( setState ).toHaveBeenLastCalledWith( { + expect( onChangeSpy ).toHaveBeenLastCalledWith( { top: '7px', right: '7px', bottom: '7px', diff --git a/packages/components/src/unit-control/test/index.tsx b/packages/components/src/unit-control/test/index.tsx index 1677ab371e3f3..423cbdce5be37 100644 --- a/packages/components/src/unit-control/test/index.tsx +++ b/packages/components/src/unit-control/test/index.tsx @@ -14,7 +14,6 @@ import { useState } from '@wordpress/element'; */ import UnitControl from '..'; import { CSS_UNITS, parseQuantityAndUnitFromRawValue } from '../utils'; -import type { UnitControlOnChangeCallback } from '../types'; const getInput = ( { isInputTypeText = false, @@ -137,11 +136,9 @@ describe( 'UnitControl', () => { describe( 'Value', () => { it( 'should update value on change', async () => { const user = userEvent.setup(); + const onChangeSpy = jest.fn(); - let state = '50px'; - const setState = jest.fn( ( value ) => ( state = value ) ); - - render( ); + render( ); const input = getInput(); await user.clear( input ); @@ -151,81 +148,85 @@ describe( 'UnitControl', () => { // - 1: clear // - 2: type '6' // - 3: type '62' - expect( setState ).toHaveBeenCalledTimes( 3 ); - expect( state ).toBe( '62px' ); + expect( onChangeSpy ).toHaveBeenCalledTimes( 3 ); + expect( onChangeSpy ).toHaveBeenLastCalledWith( + '62px', + expect.anything() + ); } ); it( 'should increment value on UP press', async () => { const user = userEvent.setup(); + const onChangeSpy = jest.fn(); - let state: string | undefined = '50px'; - const setState: UnitControlOnChangeCallback = ( nextState ) => - ( state = nextState ); - - render( ); + render( ); const input = getInput(); await user.type( input, '{ArrowUp}' ); - expect( state ).toBe( '51px' ); + expect( onChangeSpy ).toHaveBeenCalledTimes( 1 ); + expect( onChangeSpy ).toHaveBeenLastCalledWith( + '51px', + expect.anything() + ); } ); it( 'should increment value on UP + SHIFT press, with step', async () => { const user = userEvent.setup(); + const onChangeSpy = jest.fn(); - let state: string | undefined = '50px'; - const setState: UnitControlOnChangeCallback = ( nextState ) => - ( state = nextState ); - - render( ); + render( ); const input = getInput(); await user.type( input, '{Shift>}{ArrowUp}{/Shift}' ); - expect( state ).toBe( '60px' ); + expect( onChangeSpy ).toHaveBeenCalledTimes( 1 ); + expect( onChangeSpy ).toHaveBeenLastCalledWith( + '60px', + expect.anything() + ); } ); it( 'should decrement value on DOWN press', async () => { const user = userEvent.setup(); + const onChangeSpy = jest.fn(); - let state: string | number | undefined = 50; - const setState: UnitControlOnChangeCallback = ( nextState ) => - ( state = nextState ); - - render( ); + render( ); const input = getInput(); await user.type( input, '{ArrowDown}' ); - expect( state ).toBe( '49px' ); + expect( onChangeSpy ).toHaveBeenCalledTimes( 1 ); + expect( onChangeSpy ).toHaveBeenLastCalledWith( + '49px', + expect.anything() + ); } ); it( 'should decrement value on DOWN + SHIFT press, with step', async () => { const user = userEvent.setup(); + const onChangeSpy = jest.fn(); - let state: string | number | undefined = 50; - const setState: UnitControlOnChangeCallback = ( nextState ) => - ( state = nextState ); - - render( ); + render( ); const input = getInput(); await user.type( input, '{Shift>}{ArrowDown}{/Shift}' ); - expect( state ).toBe( '40px' ); + expect( onChangeSpy ).toHaveBeenCalledTimes( 1 ); + expect( onChangeSpy ).toHaveBeenLastCalledWith( + '40px', + expect.anything() + ); } ); it( 'should cancel change when ESCAPE key is pressed', async () => { const user = userEvent.setup(); - - let state: string | number | undefined = 50; - const setState: UnitControlOnChangeCallback = ( nextState ) => - ( state = nextState ); + const onChangeSpy = jest.fn(); render( ); @@ -236,12 +237,12 @@ describe( 'UnitControl', () => { await user.type( input, '300' ); expect( input.value ).toBe( '300' ); - expect( state ).toBe( 50 ); + expect( onChangeSpy ).not.toHaveBeenCalled(); await user.keyboard( '{Escape}' ); expect( input.value ).toBe( '50' ); - expect( state ).toBe( 50 ); + expect( onChangeSpy ).not.toHaveBeenCalled(); } ); it( 'should run onBlur callback when quantity input is blurred', async () => { @@ -250,16 +251,10 @@ describe( 'UnitControl', () => { const onChangeSpy = jest.fn(); const onBlurSpy = jest.fn(); - let state: string | undefined = '33%'; - const setState: UnitControlOnChangeCallback = ( nextState ) => { - onChangeSpy( nextState ); - state = nextState; - }; - render( ); @@ -269,7 +264,10 @@ describe( 'UnitControl', () => { await user.type( input, '41' ); expect( onChangeSpy ).toHaveBeenCalledTimes( 3 ); - expect( onChangeSpy ).toHaveBeenLastCalledWith( '41%' ); + expect( onChangeSpy ).toHaveBeenLastCalledWith( + '41%', + expect.anything() + ); // Clicking document.body to trigger a blur event on the input. await user.click( document.body ); @@ -282,16 +280,10 @@ describe( 'UnitControl', () => { const onChangeSpy = jest.fn(); - let state: string | undefined = '15px'; - const setState: UnitControlOnChangeCallback = ( nextState ) => { - onChangeSpy( nextState ); - state = nextState; - }; - render( ); @@ -307,7 +299,10 @@ describe( 'UnitControl', () => { // True for the test environment but in common browsers this would // be last called with '41vw' (after the call with '41px'). - expect( onChangeSpy ).toHaveBeenLastCalledWith( '41px' ); + expect( onChangeSpy ).toHaveBeenLastCalledWith( + '41px', + expect.anything() + ); } ); it( 'should update value correctly when typed and blurred when a single unit is passed', async () => { @@ -346,26 +341,30 @@ describe( 'UnitControl', () => { describe( 'Unit', () => { it( 'should update unit value on change', async () => { const user = userEvent.setup(); - - let state: string | undefined = '14rem'; - const setState: UnitControlOnChangeCallback = ( nextState ) => - ( state = nextState ); - - const spy = jest.fn(); + const onChangeSpy = jest.fn(); + const onUnitChangeSpy = jest.fn(); render( ); const select = getSelect(); await user.selectOptions( select, [ 'px' ] ); - expect( spy ).toHaveBeenCalledWith( 'px', expect.anything() ); - expect( state ).toBe( '14px' ); + expect( onUnitChangeSpy ).toHaveBeenCalledTimes( 1 ); + expect( onUnitChangeSpy ).toHaveBeenLastCalledWith( + 'px', + expect.anything() + ); + expect( onChangeSpy ).toHaveBeenCalledTimes( 1 ); + expect( onChangeSpy ).toHaveBeenLastCalledWith( + '14px', + expect.anything() + ); } ); it( 'should render customized units, if defined', () => { @@ -388,10 +387,7 @@ describe( 'UnitControl', () => { it( 'should reset value on unit change, if unit has default value', async () => { const user = userEvent.setup(); - - let state: string | number | undefined = 50; - const setState: UnitControlOnChangeCallback = ( nextState ) => - ( state = nextState ); + const onChangeSpy = jest.fn(); const units = [ { value: 'pt', label: 'pt', default: 25 }, @@ -402,27 +398,32 @@ describe( 'UnitControl', () => { ); const select = getSelect(); await user.selectOptions( select, [ 'vmax' ] ); - expect( state ).toBe( '75vmax' ); + expect( onChangeSpy ).toHaveBeenCalledTimes( 1 ); + expect( onChangeSpy ).toHaveBeenLastCalledWith( + '75vmax', + expect.anything() + ); await user.selectOptions( select, [ 'pt' ] ); - expect( state ).toBe( '25pt' ); + expect( onChangeSpy ).toHaveBeenCalledTimes( 2 ); + expect( onChangeSpy ).toHaveBeenLastCalledWith( + '25pt', + expect.anything() + ); } ); it( 'should not reset value on unit change, if disabled', async () => { const user = userEvent.setup(); - - let state: string | number | undefined = 50; - const setState: UnitControlOnChangeCallback = ( nextState ) => - ( state = nextState ); + const onChangeSpy = jest.fn(); const units = [ { value: 'pt', label: 'pt', default: 25 }, @@ -432,34 +433,39 @@ describe( 'UnitControl', () => { render( ); const select = getSelect(); await user.selectOptions( select, [ 'vmax' ] ); - expect( state ).toBe( '50vmax' ); + expect( onChangeSpy ).toHaveBeenCalledTimes( 1 ); + expect( onChangeSpy ).toHaveBeenLastCalledWith( + '50vmax', + expect.anything() + ); await user.selectOptions( select, [ 'pt' ] ); - expect( state ).toBe( '50pt' ); + expect( onChangeSpy ).toHaveBeenCalledTimes( 2 ); + expect( onChangeSpy ).toHaveBeenLastCalledWith( + '50pt', + expect.anything() + ); } ); it( 'should set correct unit if single units', async () => { const user = userEvent.setup(); - - let state: string | undefined = '50%'; - const setState: UnitControlOnChangeCallback = ( value ) => - ( state = value ); + const onChangeSpy = jest.fn(); render( ); @@ -467,7 +473,15 @@ describe( 'UnitControl', () => { await user.clear( input ); await user.type( input, '62' ); - expect( state ).toBe( '62%' ); + // 3 times: + // - 1: clear + // - 2: type '6' + // - 3: type '62' + expect( onChangeSpy ).toHaveBeenCalledTimes( 3 ); + expect( onChangeSpy ).toHaveBeenLastCalledWith( + '62%', + expect.anything() + ); } ); it( 'should update unit value when a new raw value is passed', async () => { @@ -592,7 +606,16 @@ describe( 'UnitControl', () => { 'should move focus from the input to the unit select when typing the first character of %p', async ( testUnit ) => { const user = userEvent.setup(); - render( ); + const onChangeSpy = jest.fn(); + const onUnitChangeSpy = jest.fn(); + + render( + + ); const input = getInput(); await user.clear( input ); @@ -601,6 +624,12 @@ describe( 'UnitControl', () => { expect( getSelect() ).toHaveFocus(); // The unit character was not entered in the input. expect( input ).toHaveValue( 55 ); + + expect( onChangeSpy ).toHaveBeenCalledTimes( 3 ); + expect( onChangeSpy ).toHaveBeenLastCalledWith( + '55%', + expect.anything() + ); } ); } ); From 06d06ffb3971312ee62936c99b3208c8141bc101 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Wed, 21 Jun 2023 15:13:47 +0200 Subject: [PATCH 18/19] Clarify comments about js-dom not updating select inputs when using the keyboard --- packages/components/src/box-control/test/index.tsx | 5 +++-- packages/components/src/unit-control/test/index.tsx | 10 ++++++++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/packages/components/src/box-control/test/index.tsx b/packages/components/src/box-control/test/index.tsx index b99c6264bbc56..8a861eff37e1b 100644 --- a/packages/components/src/box-control/test/index.tsx +++ b/packages/components/src/box-control/test/index.tsx @@ -298,8 +298,9 @@ describe( 'BoxControl', () => { expect( unitSelect ).toHaveFocus(); - // Due to the test environment not being a browser the units here - // are still "px". In a browser they would be expected to be "rem". + // The correct expected behavior would be for the values to have "rem" + // as their unit, but the test environment doesn't seem to change + // values on `select` elements when using the keyboard. expect( onChangeSpy ).toHaveBeenLastCalledWith( { top: '7px', right: '7px', diff --git a/packages/components/src/unit-control/test/index.tsx b/packages/components/src/unit-control/test/index.tsx index 423cbdce5be37..9a2c719c46336 100644 --- a/packages/components/src/unit-control/test/index.tsx +++ b/packages/components/src/unit-control/test/index.tsx @@ -297,8 +297,10 @@ describe( 'UnitControl', () => { // Called only once because `isPressEnterToChange` is `true`. expect( onChangeSpy ).toHaveBeenCalledTimes( 1 ); - // True for the test environment but in common browsers this would - // be last called with '41vw' (after the call with '41px'). + // The correct expected behavior would be for the `onChangeSpy` callback + // to be called twice, first with `41px` and immediately after with `41vh`, + // but the test environment doesn't seem to change values on `select` + // elements when using the keyboard. expect( onChangeSpy ).toHaveBeenLastCalledWith( '41px', expect.anything() @@ -625,6 +627,10 @@ describe( 'UnitControl', () => { // The unit character was not entered in the input. expect( input ).toHaveValue( 55 ); + // The correct expected behavior would be for onChangeSpy to be + // called 4 times, and for the last value it was called with to be + // `55${testUnit}`, but the test environment doesn't seem to change + // values on `select` elements when using the keyboard. expect( onChangeSpy ).toHaveBeenCalledTimes( 3 ); expect( onChangeSpy ).toHaveBeenLastCalledWith( '55%', From 6a96ecd142d59055ae75fed7ecba34d741965f91 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Wed, 21 Jun 2023 15:14:43 +0200 Subject: [PATCH 19/19] Update README --- packages/components/CHANGELOG.md | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index b7d711951caed..2759cc629a336 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -4,6 +4,7 @@ ### Enhancements +- `UnitControl`: Revamp support for changing unit by typing ([#39303](https://github.com/WordPress/gutenberg/pull/39303)). - `Modal`: Update corner radius to be between buttons and the site view frame, in a 2-4-8 system. ([#51254](https://github.com/WordPress/gutenberg/pull/51254)). - `ItemGroup`: Update button focus state styles to be inline with other button focus states in the editor. ([#51576](https://github.com/WordPress/gutenberg/pull/51576)). @@ -17,10 +18,6 @@ - `ClipboardButton`: Convert to TypeScript ([#51334](https://github.com/WordPress/gutenberg/pull/51334)). - `Toolbar`: Replace `reakit` dependency with `@ariakit/react` ([#51623](https://github.com/WordPress/gutenberg/pull/51623)). -### Enhancements - -- `UnitControl`: Revamp support for changing unit by typing ([#39303](https://github.com/WordPress/gutenberg/pull/39303)). - ## 25.1.0 (2023-06-07) ### Enhancements