From f943b5b6ea185a36e4a52ccd79be07bab06d354a Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Sat, 23 Apr 2022 14:32:16 -0700 Subject: [PATCH 01/28] Update `RangeControl` to play nice with revised `InputControl` --- .../components/src/range-control/index.js | 9 +--- .../components/src/range-control/utils.js | 41 ------------------- 2 files changed, 2 insertions(+), 48 deletions(-) diff --git a/packages/components/src/range-control/index.js b/packages/components/src/range-control/index.js index fb64ec8ea9b6c..3b7b896730ad8 100644 --- a/packages/components/src/range-control/index.js +++ b/packages/components/src/range-control/index.js @@ -19,7 +19,7 @@ import BaseControl from '../base-control'; import Button from '../button'; import Icon from '../icon'; import { COLORS } from '../utils'; -import { floatClamp, useControlledRangeValue } from './utils'; +import { floatClamp } from './utils'; import InputRange from './input-range'; import RangeRail from './rail'; import SimpleTooltip from './tooltip'; @@ -70,12 +70,7 @@ function RangeControl( }, ref ) { - const [ value, setValue ] = useControlledRangeValue( { - min, - max, - value: valueProp, - initial: initialPosition, - } ); + const [ value, setValue ] = useState( valueProp ?? initialPosition ?? '' ); const isResetPendent = useRef( false ); if ( step === 'any' ) { diff --git a/packages/components/src/range-control/utils.js b/packages/components/src/range-control/utils.js index e2a758a6b4f1c..255c2aaaa7d34 100644 --- a/packages/components/src/range-control/utils.js +++ b/packages/components/src/range-control/utils.js @@ -9,11 +9,6 @@ import { clamp, noop } from 'lodash'; */ import { useCallback, useRef, useEffect, useState } from '@wordpress/element'; -/** - * Internal dependencies - */ -import { useControlledState } from '../utils/hooks'; - /** * A float supported clamp function for a specific value. * @@ -31,42 +26,6 @@ export function floatClamp( value, min, max ) { return parseFloat( clamp( value, min, max ) ); } -/** - * Hook to store a clamped value, derived from props. - * - * @param {Object} settings Hook settings. - * @param {number} settings.min The minimum value. - * @param {number} settings.max The maximum value. - * @param {number} settings.value The current value. - * @param {any} settings.initial The initial value. - * - * @return {[*, Function]} The controlled value and the value setter. - */ -export function useControlledRangeValue( { - min, - max, - value: valueProp, - initial, -} ) { - const [ state, setInternalState ] = useControlledState( - floatClamp( valueProp, min, max ), - { initial, fallback: null } - ); - - const setState = useCallback( - ( nextValue ) => { - if ( nextValue === null ) { - setInternalState( null ); - } else { - setInternalState( floatClamp( nextValue, min, max ) ); - } - }, - [ min, max ] - ); - - return [ state, setState ]; -} - /** * Hook to encapsulate the debouncing "hover" to better handle the showing * and hiding of the Tooltip. From e662bdef95ffb7318cb899890e1252022e0ed266 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Sat, 23 Apr 2022 14:39:03 -0700 Subject: [PATCH 02/28] Update `UnitControl` to play nice with revised `InputControl` --- packages/components/src/unit-control/index.tsx | 4 ---- packages/components/src/unit-control/test/index.js | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/components/src/unit-control/index.tsx b/packages/components/src/unit-control/index.tsx index e97bc6ff955f6..b191cc5a4500f 100644 --- a/packages/components/src/unit-control/index.tsx +++ b/packages/components/src/unit-control/index.tsx @@ -178,10 +178,6 @@ function UnforwardedUnitControl( : undefined; const changeProps = { event, data }; - onChangeProp?.( - `${ validParsedQuantity ?? '' }${ validParsedUnit }`, - changeProps - ); onUnitChange?.( validParsedUnit, changeProps ); setUnit( validParsedUnit ); diff --git a/packages/components/src/unit-control/test/index.js b/packages/components/src/unit-control/test/index.js index d595f6171076b..615d6dcd7af84 100644 --- a/packages/components/src/unit-control/test/index.js +++ b/packages/components/src/unit-control/test/index.js @@ -215,7 +215,7 @@ describe( 'UnitControl', () => { expect( input.value ).toBe( '300px' ); expect( state ).toBe( 50 ); - user.keyboard( '{Escape}' ); + await user.keyboard( '{Escape}' ); expect( input.value ).toBe( '50' ); expect( state ).toBe( 50 ); From 00777351475b2b56dae91e60e72af0d89d242e73 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Sun, 24 Apr 2022 23:45:04 -0700 Subject: [PATCH 03/28] Restore controlled mode to `RangeControl` --- .../components/src/range-control/index.js | 38 ++++++++++--------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/packages/components/src/range-control/index.js b/packages/components/src/range-control/index.js index 3b7b896730ad8..af3f6c69fa950 100644 --- a/packages/components/src/range-control/index.js +++ b/packages/components/src/range-control/index.js @@ -18,7 +18,7 @@ import { useInstanceId } from '@wordpress/compose'; import BaseControl from '../base-control'; import Button from '../button'; import Icon from '../icon'; -import { COLORS } from '../utils'; +import { COLORS, useControlledValue } from '../utils'; import { floatClamp } from './utils'; import InputRange from './input-range'; import RangeRail from './rail'; @@ -70,8 +70,26 @@ function RangeControl( }, ref ) { - const [ value, setValue ] = useState( valueProp ?? initialPosition ?? '' ); - const isResetPendent = useRef( false ); + const isResetPendent = useRef( false ) + const [ value, setValue ] = useControlledValue( { + defaultValue: initialPosition ?? '', + value: isResetPendent.current ? undefined : valueProp, + onChange: ( nextValue ) => { + /* + * Calls onChange only when nextValue is numeric + * otherwise may queue a reset for the blur event. + */ + if ( ! isNaN( nextValue ) ) { + if ( nextValue < min || nextValue > max ) { + nextValue = floatClamp( nextValue, min, max ); + } + onChange( nextValue ); + isResetPendent.current = false; + } else if ( allowReset ) { + isResetPendent.current = true; + } + }, + } ); if ( step === 'any' ) { // The tooltip and number input field are hidden when the step is "any" @@ -121,25 +139,11 @@ function RangeControl( const handleOnRangeChange = ( event ) => { const nextValue = parseFloat( event.target.value ); setValue( nextValue ); - onChange( nextValue ); }; const handleOnChange = ( nextValue ) => { nextValue = parseFloat( nextValue ); setValue( nextValue ); - /* - * Calls onChange only when nextValue is numeric - * otherwise may queue a reset for the blur event. - */ - if ( ! isNaN( nextValue ) ) { - if ( nextValue < min || nextValue > max ) { - nextValue = floatClamp( nextValue, min, max ); - } - onChange( nextValue ); - isResetPendent.current = false; - } else if ( allowReset ) { - isResetPendent.current = true; - } }; const handleOnInputNumberBlur = () => { From 295035c67e25f4f2b460bb22804a93b1d2b00bd9 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Mon, 25 Apr 2022 11:46:38 +0200 Subject: [PATCH 04/28] Add missing ; --- packages/components/src/range-control/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/components/src/range-control/index.js b/packages/components/src/range-control/index.js index af3f6c69fa950..933d48e9798e2 100644 --- a/packages/components/src/range-control/index.js +++ b/packages/components/src/range-control/index.js @@ -70,7 +70,7 @@ function RangeControl( }, ref ) { - const isResetPendent = useRef( false ) + const isResetPendent = useRef( false ); const [ value, setValue ] = useControlledValue( { defaultValue: initialPosition ?? '', value: isResetPendent.current ? undefined : valueProp, From 739372f1e5f721e98530f2ef2007724241e24dbe Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Mon, 25 Apr 2022 13:28:55 +0200 Subject: [PATCH 05/28] Add comment after deleting `onChange` --- 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 b191cc5a4500f..3296438b04b8c 100644 --- a/packages/components/src/unit-control/index.tsx +++ b/packages/components/src/unit-control/index.tsx @@ -178,6 +178,7 @@ function UnforwardedUnitControl( : undefined; const changeProps = { event, data }; + // The `onChange` callback already gets called, no need to call it explicitely. onUnitChange?.( validParsedUnit, changeProps ); setUnit( validParsedUnit ); From cd1b4dfec583a47afb469b062c9a6a1ca933f8b0 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Mon, 25 Apr 2022 14:51:03 -0700 Subject: [PATCH 06/28] Update test of `RangeControl` to also test controlled mode --- .../src/range-control/test/index.js | 60 ++++++++++++------- 1 file changed, 40 insertions(+), 20 deletions(-) diff --git a/packages/components/src/range-control/test/index.js b/packages/components/src/range-control/test/index.js index c60f12b3b7072..6124d9bc34b7f 100644 --- a/packages/components/src/range-control/test/index.js +++ b/packages/components/src/range-control/test/index.js @@ -3,6 +3,11 @@ */ import { fireEvent, render } from '@testing-library/react'; +/** + * WordPress dependencies + */ +import { useState } from '@wordpress/element'; + /** * Internal dependencies */ @@ -15,6 +20,15 @@ const getNumberInput = ( container ) => const getResetButton = ( container ) => container.querySelector( '.components-range-control__reset' ); +function ControlledRangeControl( props ) { + const [ value, setValue ] = useState( props.value ); + const onChange = ( v ) => { + setValue( v ); + props.onChange?.( v ); + }; + return ; +} + describe( 'RangeControl', () => { describe( '#render()', () => { it( 'should trigger change callback with numeric value', () => { @@ -296,25 +310,31 @@ describe( 'RangeControl', () => { expect( spy ).toHaveBeenCalledWith( 33 ); } ); - it( 'should reset to a 50% of min/max value, of no initialPosition or value is defined', () => { - const { container } = render( - - ); - - const resetButton = getResetButton( container ); - const rangeInput = getRangeInput( container ); - const numberInput = getNumberInput( container ); - - fireEvent.click( resetButton ); - - expect( rangeInput.value ).toBe( '50' ); - expect( numberInput.value ).toBe( '' ); - } ); + it.concurrent.each( [ RangeControl, ControlledRangeControl ] )( + 'should reset to a 50% of min/max value, if no initialPosition or resetFallbackValue is defined', + ( Component ) => { + const value = + Component === ControlledRangeControl ? 89 : undefined; + const { container } = render( + + ); + + const resetButton = getResetButton( container ); + const rangeInput = getRangeInput( container ); + const numberInput = getNumberInput( container ); + + fireEvent.click( resetButton ); + + expect( rangeInput.value ).toBe( '50' ); + expect( numberInput.value ).toBe( '' ); + } + ); } ); } ); From 9a3804f4636ceba8908658ac11cc5bb9edfc5d80 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Mon, 25 Apr 2022 14:56:19 -0700 Subject: [PATCH 07/28] Remove separate onChange call from reset handling in `RangeControl` --- .../components/src/range-control/index.js | 25 ++++++------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/packages/components/src/range-control/index.js b/packages/components/src/range-control/index.js index 933d48e9798e2..6af92c87cf572 100644 --- a/packages/components/src/range-control/index.js +++ b/packages/components/src/range-control/index.js @@ -85,6 +85,14 @@ function RangeControl( } onChange( nextValue ); isResetPendent.current = false; + } else if ( nextValue === null ) { + /* + * handleOnReset has led the execution here due to lack of a + * defined resetFallbackValue. In such conditions the onChange + * callback receives undefined as that was the behavior when the + * component was stablized. + */ + onChange( undefined ); } else if ( allowReset ) { isResetPendent.current = true; } @@ -155,29 +163,12 @@ function RangeControl( const handleOnReset = () => { let resetValue = parseFloat( resetFallbackValue ); - let onChangeResetValue = resetValue; if ( isNaN( resetValue ) ) { resetValue = null; - onChangeResetValue = undefined; } setValue( resetValue ); - - /** - * Previously, this callback would always receive undefined as - * an argument. This behavior is unexpected, specifically - * when resetFallbackValue is defined. - * - * The value of undefined is not ideal. Passing it through - * to internal elements would change it from a - * controlled component to an uncontrolled component. - * - * For now, to minimize unexpected regressions, we're going to - * preserve the undefined callback argument, except when a - * resetFallbackValue is defined. - */ - onChange( onChangeResetValue ); }; const handleShowTooltip = () => setShowTooltip( true ); From 4dc4674873140ef15867ba50f8e711db33db01a5 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Mon, 25 Apr 2022 14:58:06 -0700 Subject: [PATCH 08/28] Refine RESET logic of `InputControl` reducer --- packages/components/src/input-control/reducer/reducer.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/components/src/input-control/reducer/reducer.ts b/packages/components/src/input-control/reducer/reducer.ts index 478b99873f5c5..5c808f9d53d0e 100644 --- a/packages/components/src/input-control/reducer/reducer.ts +++ b/packages/components/src/input-control/reducer/reducer.ts @@ -98,7 +98,10 @@ function inputControlStateReducer( case actions.RESET: nextState.error = null; nextState.isDirty = false; - nextState.value = action.payload.value || state.initialValue; + nextState.value = + action.payload.value === undefined + ? state.initialValue + : action.payload.value ?? ''; break; /** From 302096d5a104bdee52f61cbd86e49e67b9c28a13 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Mon, 25 Apr 2022 21:44:53 -0700 Subject: [PATCH 09/28] Simplify refined RESET logic of `InputControl` reducer --- packages/components/src/input-control/reducer/reducer.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/components/src/input-control/reducer/reducer.ts b/packages/components/src/input-control/reducer/reducer.ts index 5c808f9d53d0e..860ac052b26e8 100644 --- a/packages/components/src/input-control/reducer/reducer.ts +++ b/packages/components/src/input-control/reducer/reducer.ts @@ -98,10 +98,7 @@ function inputControlStateReducer( case actions.RESET: nextState.error = null; nextState.isDirty = false; - nextState.value = - action.payload.value === undefined - ? state.initialValue - : action.payload.value ?? ''; + nextState.value = action.payload.value ?? state.initialValue; break; /** From f3c881f03961224a86125ad42d1a284e7aecafff Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Mon, 25 Apr 2022 23:00:04 -0700 Subject: [PATCH 10/28] Restore initial position of `RangeControl` when without value --- packages/components/src/range-control/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/components/src/range-control/index.js b/packages/components/src/range-control/index.js index 6af92c87cf572..b1e3f19ab924b 100644 --- a/packages/components/src/range-control/index.js +++ b/packages/components/src/range-control/index.js @@ -72,7 +72,7 @@ function RangeControl( ) { const isResetPendent = useRef( false ); const [ value, setValue ] = useControlledValue( { - defaultValue: initialPosition ?? '', + defaultValue: initialPosition ?? null, value: isResetPendent.current ? undefined : valueProp, onChange: ( nextValue ) => { /* From 19fd10086e590bc114379180926ec4d07f970db7 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Mon, 25 Apr 2022 23:18:50 -0700 Subject: [PATCH 11/28] Differentiate state sync effect hooks by event existence --- .../src/input-control/reducer/actions.ts | 2 +- .../src/input-control/reducer/reducer.ts | 31 +++++++++++++------ .../src/input-control/reducer/state.ts | 11 ++++--- 3 files changed, 28 insertions(+), 16 deletions(-) diff --git a/packages/components/src/input-control/reducer/actions.ts b/packages/components/src/input-control/reducer/actions.ts index d66eb5778aad8..343e652b2190c 100644 --- a/packages/components/src/input-control/reducer/actions.ts +++ b/packages/components/src/input-control/reducer/actions.ts @@ -20,7 +20,7 @@ export const PRESS_UP = 'PRESS_UP'; export const RESET = 'RESET'; interface EventPayload { - event?: SyntheticEvent; + event: SyntheticEvent; } interface Action< Type, ExtraPayload = {} > { diff --git a/packages/components/src/input-control/reducer/reducer.ts b/packages/components/src/input-control/reducer/reducer.ts index 860ac052b26e8..4c7de7501c46d 100644 --- a/packages/components/src/input-control/reducer/reducer.ts +++ b/packages/components/src/input-control/reducer/reducer.ts @@ -54,6 +54,15 @@ function inputControlStateReducer( return ( state, action ) => { const nextState = { ...state }; + // Updates state and returns early when there's no action type. These + // are controlled updates and need no exposure to additional reducers. + if ( ! ( 'type' in action ) ) { + let { value = state.value } = action; + value ??= ''; + if ( value !== '' ) value = `${ value }`; + return { ...state, value }; + } + switch ( action.type ) { /** * Keyboard events @@ -109,10 +118,6 @@ function inputControlStateReducer( break; } - if ( action.payload.event ) { - nextState._event = action.payload.event; - } - /** * Send the nextState + action to the composedReducers via * this "bridge" mechanism. This allows external stateReducers @@ -160,6 +165,7 @@ export function useInputControlStateReducer( event.persist(); } + refEvent.current = event; dispatch( { type, payload: { value: nextValue, event }, @@ -178,12 +184,14 @@ export function useInputControlStateReducer( event.persist(); } + refEvent.current = event; dispatch( { type, payload: { event } } ); }; const createDragEvent = ( type: actions.DragEventAction[ 'type' ] ) => ( payload: actions.DragEventAction[ 'payload' ] ) => { + refEvent.current = payload.event; dispatch( { type, payload } ); }; @@ -191,8 +199,10 @@ export function useInputControlStateReducer( * Actions for the reducer */ const change = createChangeEvent( actions.CHANGE ); - const invalidate = ( error: unknown, event: SyntheticEvent ) => + const invalidate = ( error: unknown, event: SyntheticEvent ) => { + refEvent.current = event; dispatch( { type: actions.INVALIDATE, payload: { error, event } } ); + }; const reset = createChangeEvent( actions.RESET ); const commit = createChangeEvent( actions.COMMIT ); @@ -206,31 +216,32 @@ export function useInputControlStateReducer( const currentState = useRef( state ); const currentValueProp = useRef( initialState.value ); + const refEvent = useRef< SyntheticEvent | null >( null ); useLayoutEffect( () => { currentState.current = state; currentValueProp.current = initialState.value; } ); useLayoutEffect( () => { if ( + refEvent.current && state.value !== currentValueProp.current && ! currentState.current.isDirty ) { onChangeHandler( state.value ?? '', { - event: currentState.current._event as + event: refEvent.current as | ChangeEvent< HTMLInputElement > | PointerEvent< HTMLInputElement >, } ); + refEvent.current = null; } }, [ state.value ] ); useLayoutEffect( () => { if ( + ! refEvent.current && initialState.value !== currentState.current.value && ! currentState.current.isDirty ) { - reset( - initialState.value, - currentState.current._event as SyntheticEvent - ); + dispatch( { value: initialState.value } ); } }, [ initialState.value ] ); diff --git a/packages/components/src/input-control/reducer/state.ts b/packages/components/src/input-control/reducer/state.ts index be7dd3547300b..aacf31cc04b35 100644 --- a/packages/components/src/input-control/reducer/state.ts +++ b/packages/components/src/input-control/reducer/state.ts @@ -9,22 +9,23 @@ import type { Reducer } from 'react'; import type { InputAction } from './actions'; export interface InputState { - _event: Event | {}; error: unknown; - initialValue?: string; + initialValue: string; isDirty: boolean; isDragEnabled: boolean; isDragging: boolean; isPressEnterToChange: boolean; - value?: string; + value: string; } -export type StateReducer = Reducer< InputState, InputAction >; +export type StateReducer = Reducer< + InputState, + InputAction | Partial< InputState > +>; export const initialStateReducer: StateReducer = ( state: InputState ) => state; export const initialInputControlState: InputState = { - _event: {}, error: null, initialValue: '', isDirty: false, From 68a02ea9469409699d66867d0aa865e3d21cd95a Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Mon, 25 Apr 2022 23:20:43 -0700 Subject: [PATCH 12/28] Add and use type `SecondaryReducer` --- packages/components/src/input-control/reducer/state.ts | 1 + packages/components/src/unit-control/index.tsx | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/components/src/input-control/reducer/state.ts b/packages/components/src/input-control/reducer/state.ts index aacf31cc04b35..c7201d12a9df7 100644 --- a/packages/components/src/input-control/reducer/state.ts +++ b/packages/components/src/input-control/reducer/state.ts @@ -22,6 +22,7 @@ export type StateReducer = Reducer< InputState, InputAction | Partial< InputState > >; +export type SecondaryReducer = Reducer< InputState, InputAction >; export const initialStateReducer: StateReducer = ( state: InputState ) => state; diff --git a/packages/components/src/unit-control/index.tsx b/packages/components/src/unit-control/index.tsx index 3296438b04b8c..089155b9d2ffd 100644 --- a/packages/components/src/unit-control/index.tsx +++ b/packages/components/src/unit-control/index.tsx @@ -34,7 +34,7 @@ import { } from './utils'; import { useControlledState } from '../utils/hooks'; import type { UnitControlProps, UnitControlOnChangeCallback } from './types'; -import type { StateReducer } from '../input-control/reducer/state'; +import type { SecondaryReducer } from '../input-control/reducer/state'; function UnforwardedUnitControl( unitControlProps: WordPressComponentProps< @@ -206,7 +206,7 @@ function UnforwardedUnitControl( * @param action Action triggering state change * @return The updated state to apply to InputControl */ - const unitControlStateReducer: StateReducer = ( state, action ) => { + const unitControlStateReducer: SecondaryReducer = ( state, action ) => { const nextState = { ...state }; /* @@ -226,7 +226,7 @@ function UnforwardedUnitControl( return nextState; }; - let stateReducer: StateReducer = unitControlStateReducer; + let stateReducer: SecondaryReducer = unitControlStateReducer; if ( stateReducerProp ) { stateReducer = ( state, action ) => { const baseState = unitControlStateReducer( state, action ); From d6b43bb7436e0ee76621fdc4d3cf3360ffce9386 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Mon, 25 Apr 2022 23:23:43 -0700 Subject: [PATCH 13/28] Cleanup legacy `event.perist()` --- .../src/input-control/reducer/reducer.ts | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/packages/components/src/input-control/reducer/reducer.ts b/packages/components/src/input-control/reducer/reducer.ts index 4c7de7501c46d..d5e7273fe047d 100644 --- a/packages/components/src/input-control/reducer/reducer.ts +++ b/packages/components/src/input-control/reducer/reducer.ts @@ -156,15 +156,6 @@ export function useInputControlStateReducer( nextValue: actions.ChangeEventAction[ 'payload' ][ 'value' ], event: actions.ChangeEventAction[ 'payload' ][ 'event' ] ) => { - /** - * Persist allows for the (Synthetic) event to be used outside of - * this function call. - * https://reactjs.org/docs/events.html#event-pooling - */ - if ( event && event.persist ) { - event.persist(); - } - refEvent.current = event; dispatch( { type, @@ -175,15 +166,6 @@ export function useInputControlStateReducer( const createKeyEvent = ( type: actions.KeyEventAction[ 'type' ] ) => ( event: actions.KeyEventAction[ 'payload' ][ 'event' ] ) => { - /** - * Persist allows for the (Synthetic) event to be used outside of - * this function call. - * https://reactjs.org/docs/events.html#event-pooling - */ - if ( event && event.persist ) { - event.persist(); - } - refEvent.current = event; dispatch( { type, payload: { event } } ); }; From b3e9d887d1ad1add1d6091309853246ec2e67850 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Tue, 26 Apr 2022 13:58:39 -0700 Subject: [PATCH 14/28] Simplify update from props in reducer Co-authored-by: Lena Morita --- packages/components/src/input-control/reducer/reducer.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/components/src/input-control/reducer/reducer.ts b/packages/components/src/input-control/reducer/reducer.ts index d5e7273fe047d..9e56ca2d04d84 100644 --- a/packages/components/src/input-control/reducer/reducer.ts +++ b/packages/components/src/input-control/reducer/reducer.ts @@ -57,10 +57,10 @@ function inputControlStateReducer( // Updates state and returns early when there's no action type. These // are controlled updates and need no exposure to additional reducers. if ( ! ( 'type' in action ) ) { - let { value = state.value } = action; - value ??= ''; - if ( value !== '' ) value = `${ value }`; - return { ...state, value }; + return { + ...state, + value: `${ action.value ?? '' }`, + }; } switch ( action.type ) { From 547a7c73ec4695f786cc6ec9b473e919bc32a60e Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Tue, 26 Apr 2022 13:43:01 -0700 Subject: [PATCH 15/28] Ensure event is cleared after drag actions --- packages/components/src/input-control/reducer/reducer.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/components/src/input-control/reducer/reducer.ts b/packages/components/src/input-control/reducer/reducer.ts index 9e56ca2d04d84..3b3fcaad3115f 100644 --- a/packages/components/src/input-control/reducer/reducer.ts +++ b/packages/components/src/input-control/reducer/reducer.ts @@ -204,8 +204,9 @@ export function useInputControlStateReducer( currentValueProp.current = initialState.value; } ); useLayoutEffect( () => { + if ( ! refEvent.current ) return; + if ( - refEvent.current && state.value !== currentValueProp.current && ! currentState.current.isDirty ) { @@ -214,9 +215,9 @@ export function useInputControlStateReducer( | ChangeEvent< HTMLInputElement > | PointerEvent< HTMLInputElement >, } ); - refEvent.current = null; } - }, [ state.value ] ); + refEvent.current = null; + }, [ state.value, state.isDragging ] ); useLayoutEffect( () => { if ( ! refEvent.current && From 89affab7b9375911a3cbaa977e32679a77f567f2 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Tue, 26 Apr 2022 13:48:47 -0700 Subject: [PATCH 16/28] Avoid declaration of potential unused variable --- packages/components/src/input-control/reducer/reducer.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/components/src/input-control/reducer/reducer.ts b/packages/components/src/input-control/reducer/reducer.ts index 3b3fcaad3115f..de8eb1312b068 100644 --- a/packages/components/src/input-control/reducer/reducer.ts +++ b/packages/components/src/input-control/reducer/reducer.ts @@ -52,8 +52,6 @@ function inputControlStateReducer( composedStateReducers: StateReducer ): StateReducer { return ( state, action ) => { - const nextState = { ...state }; - // Updates state and returns early when there's no action type. These // are controlled updates and need no exposure to additional reducers. if ( ! ( 'type' in action ) ) { @@ -62,6 +60,7 @@ function inputControlStateReducer( value: `${ action.value ?? '' }`, }; } + const nextState = { ...state }; switch ( action.type ) { /** From 06d16fd080f89eda76f396c65e952eddf1834211 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Thu, 28 Apr 2022 10:13:10 -0700 Subject: [PATCH 17/28] Add more reset unit tests for `RangeControl` --- .../src/range-control/test/index.js | 27 +++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/packages/components/src/range-control/test/index.js b/packages/components/src/range-control/test/index.js index 6124d9bc34b7f..803b0dd9c92ff 100644 --- a/packages/components/src/range-control/test/index.js +++ b/packages/components/src/range-control/test/index.js @@ -288,14 +288,31 @@ describe( 'RangeControl', () => { } ); describe( 'reset', () => { - it( 'should reset to a custom fallback value, defined by a parent component', () => { + it.concurrent.each( [ + [ + 'initialPosition if it is defined', + { initialPosition: 21 }, + [ '21', undefined ], + ], + [ + 'resetFallbackValue if it is defined', + { resetFallbackValue: 34 }, + [ '34', 34 ], + ], + [ + 'resetFallbackValue if both it and initialPosition are defined', + { initialPosition: 21, resetFallbackValue: 34 }, + [ '34', 34 ], + ], + ] )( 'should reset to %s', ( ...all ) => { + const [ , propsForReset, [ expectedValue, expectedChange ] ] = all; const spy = jest.fn(); const { container } = render( ); @@ -305,9 +322,9 @@ describe( 'RangeControl', () => { fireEvent.click( resetButton ); - expect( rangeInput.value ).toBe( '33' ); - expect( numberInput.value ).toBe( '33' ); - expect( spy ).toHaveBeenCalledWith( 33 ); + expect( rangeInput.value ).toBe( expectedValue ); + expect( numberInput.value ).toBe( expectedValue ); + expect( spy ).toHaveBeenCalledWith( expectedChange ); } ); it.concurrent.each( [ RangeControl, ControlledRangeControl ] )( From ce0ac420f22da5afc8b750e12e850f7e4dc49110 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Thu, 28 Apr 2022 10:01:43 -0700 Subject: [PATCH 18/28] Run `RangeControl` unit test in both controlled/uncontrolled modes --- .../src/range-control/test/index.js | 101 ++++++++---------- 1 file changed, 44 insertions(+), 57 deletions(-) diff --git a/packages/components/src/range-control/test/index.js b/packages/components/src/range-control/test/index.js index 803b0dd9c92ff..634db9bc8f82d 100644 --- a/packages/components/src/range-control/test/index.js +++ b/packages/components/src/range-control/test/index.js @@ -29,14 +29,17 @@ function ControlledRangeControl( props ) { return ; } -describe( 'RangeControl', () => { +describe.each( [ + [ 'uncontrolled', RangeControl ], + [ 'controlled', ControlledRangeControl ], +] )( 'RangeControl %s', ( ...modeAndComponent ) => { + const [ , Component ] = modeAndComponent; + describe( '#render()', () => { it( 'should trigger change callback with numeric value', () => { const onChange = jest.fn(); - const { container } = render( - - ); + const { container } = render( ); const rangeInput = getRangeInput( container ); const numberInput = getNumberInput( container ); @@ -53,10 +56,7 @@ describe( 'RangeControl', () => { it( 'should render with icons', () => { const { container } = render( - + ); const beforeIcon = container.querySelector( @@ -73,7 +73,7 @@ describe( 'RangeControl', () => { describe( 'validation', () => { it( 'should not apply if new value is lower than minimum', () => { - const { container } = render( ); + const { container } = render( ); const rangeInput = getRangeInput( container ); const numberInput = getNumberInput( container ); @@ -85,7 +85,7 @@ describe( 'RangeControl', () => { } ); it( 'should not apply if new value is greater than maximum', () => { - const { container } = render( ); + const { container } = render( ); const rangeInput = getRangeInput( container ); const numberInput = getNumberInput( container ); @@ -99,7 +99,7 @@ describe( 'RangeControl', () => { it( 'should not call onChange if new value is invalid', () => { const onChange = jest.fn(); const { container } = render( - + ); const numberInput = getNumberInput( container ); @@ -113,7 +113,7 @@ describe( 'RangeControl', () => { it( 'should keep invalid values in number input until loss of focus', () => { const onChange = jest.fn(); const { container } = render( - + ); const rangeInput = getRangeInput( container ); @@ -132,7 +132,7 @@ describe( 'RangeControl', () => { it( 'should validate when provided a max or min of zero', () => { const { container } = render( - + ); const rangeInput = getRangeInput( container ); @@ -147,7 +147,7 @@ describe( 'RangeControl', () => { it( 'should validate when min and max are negative', () => { const { container } = render( - + ); const rangeInput = getRangeInput( container ); @@ -168,11 +168,7 @@ describe( 'RangeControl', () => { it( 'should take into account the step starting from min', () => { const onChange = jest.fn(); const { container } = render( - + ); const rangeInput = getRangeInput( container ); @@ -193,9 +189,7 @@ describe( 'RangeControl', () => { describe( 'initialPosition / value', () => { it( 'should render initial rendered value of 50% of min/max, if no initialPosition or value is defined', () => { - const { container } = render( - - ); + const { container } = render( ); const rangeInput = getRangeInput( container ); @@ -204,7 +198,7 @@ describe( 'RangeControl', () => { it( 'should render initialPosition if no value is provided', () => { const { container } = render( - + ); const rangeInput = getRangeInput( container ); @@ -214,7 +208,7 @@ describe( 'RangeControl', () => { it( 'should render value instead of initialPosition is provided', () => { const { container } = render( - + ); const rangeInput = getRangeInput( container ); @@ -225,7 +219,7 @@ describe( 'RangeControl', () => { describe( 'input field', () => { it( 'should render an input field by default', () => { - const { container } = render( ); + const { container } = render( ); const numberInput = getNumberInput( container ); @@ -234,7 +228,7 @@ describe( 'RangeControl', () => { it( 'should not render an input field, if disabled', () => { const { container } = render( - + ); const numberInput = getNumberInput( container ); @@ -243,7 +237,7 @@ describe( 'RangeControl', () => { } ); it( 'should render a zero value into input range and field', () => { - const { container } = render( ); + const { container } = render( ); const rangeInput = getRangeInput( container ); const numberInput = getNumberInput( container ); @@ -253,7 +247,7 @@ describe( 'RangeControl', () => { } ); it( 'should update both field and range on change', () => { - const { container } = render( ); + const { container } = render( ); const rangeInput = getRangeInput( container ); const numberInput = getNumberInput( container ); @@ -272,7 +266,7 @@ describe( 'RangeControl', () => { } ); it( 'should reset input values if next value is removed', () => { - const { container } = render( ); + const { container } = render( ); const rangeInput = getRangeInput( container ); const numberInput = getNumberInput( container ); @@ -308,8 +302,7 @@ describe( 'RangeControl', () => { const [ , propsForReset, [ expectedValue, expectedChange ] ] = all; const spy = jest.fn(); const { container } = render( - { expect( spy ).toHaveBeenCalledWith( expectedChange ); } ); - it.concurrent.each( [ RangeControl, ControlledRangeControl ] )( - 'should reset to a 50% of min/max value, if no initialPosition or resetFallbackValue is defined', - ( Component ) => { - const value = - Component === ControlledRangeControl ? 89 : undefined; - const { container } = render( - - ); - - const resetButton = getResetButton( container ); - const rangeInput = getRangeInput( container ); - const numberInput = getNumberInput( container ); - - fireEvent.click( resetButton ); - - expect( rangeInput.value ).toBe( '50' ); - expect( numberInput.value ).toBe( '' ); - } - ); + it( 'should reset to a 50% of min/max value, of no initialPosition or value is defined', () => { + const { container } = render( + + ); + + const resetButton = getResetButton( container ); + const rangeInput = getRangeInput( container ); + const numberInput = getNumberInput( container ); + + fireEvent.click( resetButton ); + + expect( rangeInput.value ).toBe( '50' ); + expect( numberInput.value ).toBe( '' ); + } ); } ); } ); From 3800d28df1a8db1bb3667a92fdef0fb296023f81 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Sun, 1 May 2022 21:32:06 -0700 Subject: [PATCH 19/28] =?UTF-8?q?Make=20=E2=80=9Ckeep=20invaid=20values?= =?UTF-8?q?=E2=80=9D=20test=20async?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/components/src/range-control/test/index.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/components/src/range-control/test/index.js b/packages/components/src/range-control/test/index.js index 634db9bc8f82d..daf9c8009b45b 100644 --- a/packages/components/src/range-control/test/index.js +++ b/packages/components/src/range-control/test/index.js @@ -1,7 +1,7 @@ /** * External dependencies */ -import { fireEvent, render } from '@testing-library/react'; +import { fireEvent, render, waitFor } from '@testing-library/react'; /** * WordPress dependencies @@ -110,7 +110,7 @@ describe.each( [ expect( onChange ).not.toHaveBeenCalled(); } ); - it( 'should keep invalid values in number input until loss of focus', () => { + it( 'should keep invalid values in number input until loss of focus', async () => { const onChange = jest.fn(); const { container } = render( @@ -122,7 +122,7 @@ describe.each( [ numberInput.focus(); fireEvent.change( numberInput, { target: { value: '-1.1' } } ); - expect( numberInput.value ).toBe( '-1.1' ); + await waitFor( () => expect( numberInput.value ).toBe( '-1.1' ) ); expect( rangeInput.value ).toBe( '-1' ); fireEvent.blur( numberInput ); From ff6e10a8f95f2616bf319c286f6e2ce1aa4e6d74 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Sun, 1 May 2022 21:38:20 -0700 Subject: [PATCH 20/28] Prevent interference of value entry in number input --- .../components/src/range-control/index.js | 50 ++++++++----------- .../components/src/range-control/utils.js | 37 ++++++++++++++ 2 files changed, 59 insertions(+), 28 deletions(-) diff --git a/packages/components/src/range-control/index.js b/packages/components/src/range-control/index.js index b1e3f19ab924b..fa00d50d3f1f3 100644 --- a/packages/components/src/range-control/index.js +++ b/packages/components/src/range-control/index.js @@ -19,7 +19,7 @@ import BaseControl from '../base-control'; import Button from '../button'; import Icon from '../icon'; import { COLORS, useControlledValue } from '../utils'; -import { floatClamp } from './utils'; +import { useUnimpededRangedNumberEntry } from './utils'; import InputRange from './input-range'; import RangeRail from './rail'; import SimpleTooltip from './tooltip'; @@ -73,29 +73,17 @@ function RangeControl( const isResetPendent = useRef( false ); const [ value, setValue ] = useControlledValue( { defaultValue: initialPosition ?? null, - value: isResetPendent.current ? undefined : valueProp, + value: valueProp, onChange: ( nextValue ) => { - /* - * Calls onChange only when nextValue is numeric - * otherwise may queue a reset for the blur event. - */ - if ( ! isNaN( nextValue ) ) { - if ( nextValue < min || nextValue > max ) { - nextValue = floatClamp( nextValue, min, max ); - } - onChange( nextValue ); - isResetPendent.current = false; - } else if ( nextValue === null ) { + if ( nextValue === null ) { /* - * handleOnReset has led the execution here due to lack of a - * defined resetFallbackValue. In such conditions the onChange - * callback receives undefined as that was the behavior when the - * component was stablized. + * The value is reset without a resetFallbackValue. In such + * conditions the onChange callback receives undefined as that + * was the behavior when the component was stablized. */ - onChange( undefined ); - } else if ( allowReset ) { - isResetPendent.current = true; + nextValue = undefined; } + onChange( nextValue ); }, } ); @@ -149,10 +137,19 @@ function RangeControl( setValue( nextValue ); }; - const handleOnChange = ( nextValue ) => { - nextValue = parseFloat( nextValue ); - setValue( nextValue ); - }; + const someNumberInputProps = useUnimpededRangedNumberEntry( { + max, + min, + value: inputSliderValue, + onChange: ( nextValue ) => { + if ( ! isNaN( nextValue ) ) { + setValue( nextValue ); + isResetPendent.current = false; + } else if ( allowReset ) { + isResetPendent.current = true; + } + }, + } ); const handleOnInputNumberBlur = () => { if ( isResetPendent.current ) { @@ -275,13 +272,10 @@ function RangeControl( disabled={ disabled } inputMode="decimal" isShiftStepEnabled={ isShiftStepEnabled } - max={ max } - min={ min } onBlur={ handleOnInputNumberBlur } - onChange={ handleOnChange } shiftStep={ shiftStep } step={ step } - value={ inputSliderValue } + { ...someNumberInputProps } /> ) } { allowReset && ( diff --git a/packages/components/src/range-control/utils.js b/packages/components/src/range-control/utils.js index 255c2aaaa7d34..506b32f514797 100644 --- a/packages/components/src/range-control/utils.js +++ b/packages/components/src/range-control/utils.js @@ -26,6 +26,43 @@ export function floatClamp( value, min, max ) { return parseFloat( clamp( value, min, max ) ); } +/** + * Enables entry of out-of-range and invalid values that diverge from state. + * + * @param {Object} props Props + * @param {number|null} props.value Incoming value. + * @param {number} props.max Maximum valid value. + * @param {number} props.min Minimum valid value. + * @param {(next: number) => void} props.onChange Callback for changes. + * + * @return {Object} Assorted props for the input. + */ +export function useUnimpededRangedNumberEntry( { max, min, onChange, value } ) { + const ref = useRef(); + const isDiverging = useRef( false ); + /** @type {import('../input-control/types').InputChangeCallback}*/ + const changeHandler = ( next ) => { + next = parseFloat( next ); + const isOverflow = next < min || next > max; + isDiverging.current = isNaN( next ) || isOverflow; + if ( isOverflow ) { + next = Math.max( min, Math.min( max, next ) ); + } + onChange( next ); + }; + useEffect( () => { + if ( ref.current && isDiverging.current ) { + const input = ref.current; + const entry = input.value; + const { defaultView } = input.ownerDocument; + defaultView.requestAnimationFrame( () => ( input.value = entry ) ); + isDiverging.current = false; + } + }, [ value ] ); + + return { max, min, ref, value, onChange: changeHandler }; +} + /** * Hook to encapsulate the debouncing "hover" to better handle the showing * and hiding of the Tooltip. From e427a0e79217c79b8f7fff008eae8832509b2a50 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Sun, 1 May 2022 21:42:27 -0700 Subject: [PATCH 21/28] Remove unused `floatClamp` function --- .../components/src/range-control/utils.js | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/packages/components/src/range-control/utils.js b/packages/components/src/range-control/utils.js index 506b32f514797..08152fcc19c1f 100644 --- a/packages/components/src/range-control/utils.js +++ b/packages/components/src/range-control/utils.js @@ -2,30 +2,13 @@ /** * External dependencies */ -import { clamp, noop } from 'lodash'; +import { noop } from 'lodash'; /** * WordPress dependencies */ import { useCallback, useRef, useEffect, useState } from '@wordpress/element'; -/** - * A float supported clamp function for a specific value. - * - * @param {number|null} value The value to clamp. - * @param {number} min The minimum value. - * @param {number} max The maximum value. - * - * @return {number} A (float) number - */ -export function floatClamp( value, min, max ) { - if ( typeof value !== 'number' ) { - return null; - } - - return parseFloat( clamp( value, min, max ) ); -} - /** * Enables entry of out-of-range and invalid values that diverge from state. * From 38c0158884ee5bf313a0a6c1d4ef92e73eccaf24 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Sun, 1 May 2022 22:26:33 -0700 Subject: [PATCH 22/28] Fix reset to `initialPosition` --- .../components/src/range-control/index.js | 26 +++++++++---------- packages/components/src/range-control/rail.js | 4 +-- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/packages/components/src/range-control/index.js b/packages/components/src/range-control/index.js index fa00d50d3f1f3..9908e23393f29 100644 --- a/packages/components/src/range-control/index.js +++ b/packages/components/src/range-control/index.js @@ -111,15 +111,15 @@ function RangeControl( const isThumbFocused = ! disabled && isFocused; const isValueReset = value === null; - const currentValue = value !== undefined ? value : currentInput; + const usedValue = isValueReset + ? resetFallbackValue ?? initialPosition + : value ?? currentInput; - const inputSliderValue = isValueReset ? '' : currentValue; - - const rangeFillValue = isValueReset ? ( max - min ) / 2 + min : value; - - const calculatedFillValue = ( ( value - min ) / ( max - min ) ) * 100; - const fillValue = isValueReset ? 50 : calculatedFillValue; - const fillValueOffset = `${ clamp( fillValue, 0, 100 ) }%`; + const fillPercent = `${ + usedValue === null || usedValue === undefined + ? 50 + : ( ( clamp( usedValue, min, max ) - min ) / ( max - min ) ) * 100 + }%`; const classes = classnames( 'components-range-control', className ); @@ -140,7 +140,7 @@ function RangeControl( const someNumberInputProps = useUnimpededRangedNumberEntry( { max, min, - value: inputSliderValue, + value: usedValue ?? '', onChange: ( nextValue ) => { if ( ! isNaN( nextValue ) ) { setValue( nextValue ); @@ -184,7 +184,7 @@ function RangeControl( }; const offsetStyle = { - [ isRTL() ? 'right' : 'left' ]: fillValueOffset, + [ isRTL() ? 'right' : 'left' ]: fillPercent, }; return ( @@ -222,7 +222,7 @@ function RangeControl( onMouseLeave={ onMouseLeave } ref={ setRef } step={ step } - value={ inputSliderValue } + value={ usedValue ?? '' } /> diff --git a/packages/components/src/range-control/rail.js b/packages/components/src/range-control/rail.js index 34caa88a73622..39496bfc989f4 100644 --- a/packages/components/src/range-control/rail.js +++ b/packages/components/src/range-control/rail.js @@ -16,7 +16,7 @@ export default function RangeRail( { min = 0, max = 100, step = 1, - value = 0, + value, ...restProps } ) { return ( @@ -29,7 +29,7 @@ export default function RangeRail( { min={ min } max={ max } step={ step } - value={ value } + value={ value ?? ( max - min ) / 2 + min } /> ) } From 8930905b29a4a0ebe3b516e7c371e2f7e5bf2a18 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Mon, 2 May 2022 11:05:02 -0700 Subject: [PATCH 23/28] Fix a couple tests for controlled `RangeControl` --- packages/components/src/range-control/test/index.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/components/src/range-control/test/index.js b/packages/components/src/range-control/test/index.js index daf9c8009b45b..5c1c41e48521b 100644 --- a/packages/components/src/range-control/test/index.js +++ b/packages/components/src/range-control/test/index.js @@ -33,7 +33,7 @@ describe.each( [ [ 'uncontrolled', RangeControl ], [ 'controlled', ControlledRangeControl ], ] )( 'RangeControl %s', ( ...modeAndComponent ) => { - const [ , Component ] = modeAndComponent; + const [ mode, Component ] = modeAndComponent; describe( '#render()', () => { it( 'should trigger change callback with numeric value', () => { @@ -306,6 +306,7 @@ describe.each( [ allowReset={ true } onChange={ spy } { ...propsForReset } + value={ mode === 'controlled' ? 89 : undefined } /> ); @@ -328,6 +329,7 @@ describe.each( [ max={ 100 } allowReset={ true } resetFallbackValue={ undefined } + value={ mode === 'controlled' ? 89 : undefined } /> ); From cb0074948782cf7e499325b366fce8f6a9955d44 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Mon, 2 May 2022 11:07:59 -0700 Subject: [PATCH 24/28] Fix `RangeControl` reset --- .../components/src/range-control/index.js | 35 +++++++++---------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/packages/components/src/range-control/index.js b/packages/components/src/range-control/index.js index 9908e23393f29..4018d37cfadc0 100644 --- a/packages/components/src/range-control/index.js +++ b/packages/components/src/range-control/index.js @@ -18,7 +18,7 @@ import { useInstanceId } from '@wordpress/compose'; import BaseControl from '../base-control'; import Button from '../button'; import Icon from '../icon'; -import { COLORS, useControlledValue } from '../utils'; +import { COLORS, useControlledState } from '../utils'; import { useUnimpededRangedNumberEntry } from './utils'; import InputRange from './input-range'; import RangeRail from './rail'; @@ -71,20 +71,8 @@ function RangeControl( ref ) { const isResetPendent = useRef( false ); - const [ value, setValue ] = useControlledValue( { - defaultValue: initialPosition ?? null, - value: valueProp, - onChange: ( nextValue ) => { - if ( nextValue === null ) { - /* - * The value is reset without a resetFallbackValue. In such - * conditions the onChange callback receives undefined as that - * was the behavior when the component was stablized. - */ - nextValue = undefined; - } - onChange( nextValue ); - }, + const [ value, setValue ] = useControlledState( valueProp, { + fallback: null, } ); if ( step === 'any' ) { @@ -135,6 +123,7 @@ function RangeControl( const handleOnRangeChange = ( event ) => { const nextValue = parseFloat( event.target.value ); setValue( nextValue ); + onChange( nextValue ); }; const someNumberInputProps = useUnimpededRangedNumberEntry( { @@ -144,6 +133,7 @@ function RangeControl( onChange: ( nextValue ) => { if ( ! isNaN( nextValue ) ) { setValue( nextValue ); + onChange( nextValue ); isResetPendent.current = false; } else if ( allowReset ) { isResetPendent.current = true; @@ -159,13 +149,20 @@ function RangeControl( }; const handleOnReset = () => { - let resetValue = parseFloat( resetFallbackValue ); + const resetValue = parseFloat( resetFallbackValue ); if ( isNaN( resetValue ) ) { - resetValue = null; + setValue( null ); + /* + * If the value is reset without a resetFallbackValue, the onChange + * callback receives undefined as that was the behavior when the + * component was stablized. + */ + onChange( undefined ); + } else { + setValue( resetValue ); + onChange( resetValue ); } - - setValue( resetValue ); }; const handleShowTooltip = () => setShowTooltip( true ); From fa2723dd3f6ffe8d0501815d27ea52e4048cd227 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Mon, 2 May 2022 11:13:04 -0700 Subject: [PATCH 25/28] =?UTF-8?q?Ensure=20`InputControl`=E2=80=99s=20state?= =?UTF-8?q?=20syncing=20works=20after=20focus=20changes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/components/src/input-control/input-field.tsx | 3 ++- packages/components/src/input-control/reducer/reducer.ts | 6 ++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/components/src/input-control/input-field.tsx b/packages/components/src/input-control/input-field.tsx index 744f7f93c79dc..7207a337613fd 100644 --- a/packages/components/src/input-control/input-field.tsx +++ b/packages/components/src/input-control/input-field.tsx @@ -73,7 +73,8 @@ function InputField( value: valueProp, isPressEnterToChange, }, - onChange + onChange, + isFocused ); const { value, isDragging, isDirty } = state; diff --git a/packages/components/src/input-control/reducer/reducer.ts b/packages/components/src/input-control/reducer/reducer.ts index de8eb1312b068..ff9813bc81e4c 100644 --- a/packages/components/src/input-control/reducer/reducer.ts +++ b/packages/components/src/input-control/reducer/reducer.ts @@ -139,12 +139,14 @@ function inputControlStateReducer( * @param stateReducer An external state reducer. * @param initialState The initial state for the reducer. * @param onChangeHandler A handler for the onChange event. + * @param isFocused Focus state. * @return State, dispatch, and a collection of actions. */ export function useInputControlStateReducer( stateReducer: StateReducer = initialStateReducer, initialState: Partial< InputState > = initialInputControlState, - onChangeHandler: InputChangeCallback + onChangeHandler: InputChangeCallback, + isFocused: Boolean ) { const [ state, dispatch ] = useReducer< StateReducer >( inputControlStateReducer( stateReducer ), @@ -216,7 +218,7 @@ export function useInputControlStateReducer( } ); } refEvent.current = null; - }, [ state.value, state.isDragging ] ); + }, [ state.value, state.isDragging, isFocused ] ); useLayoutEffect( () => { if ( ! refEvent.current && From 085fd03747e48e44cd45811f4d30a1b41fcb539e Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Tue, 10 May 2022 09:23:07 -0700 Subject: [PATCH 26/28] Comment --- packages/components/src/range-control/utils.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/components/src/range-control/utils.js b/packages/components/src/range-control/utils.js index 08152fcc19c1f..ec228e7d0daa2 100644 --- a/packages/components/src/range-control/utils.js +++ b/packages/components/src/range-control/utils.js @@ -33,6 +33,12 @@ export function useUnimpededRangedNumberEntry( { max, min, onChange, value } ) { } onChange( next ); }; + // When the value entered in the input is out of range then a clamped value + // is sent through onChange and that goes on to update the input. In such + // circumstances this effect overwrites the input value with the entered + // value to avoid interfering with typing. E.g. Without this effect, if + // `min` is 20 and the user intends to type 25, as soon as 2 is typed the + // input will update to 20 and likely lead to an entry of 205. useEffect( () => { if ( ref.current && isDiverging.current ) { const input = ref.current; From 29634d72958674c0993aa88cbb758e1e1cc0ca9a Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Tue, 10 May 2022 09:27:25 -0700 Subject: [PATCH 27/28] Ignore NaN values in `useUnimpededRangedNumberEntry` --- packages/components/src/range-control/utils.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/components/src/range-control/utils.js b/packages/components/src/range-control/utils.js index ec228e7d0daa2..aab35cf09c0f6 100644 --- a/packages/components/src/range-control/utils.js +++ b/packages/components/src/range-control/utils.js @@ -26,9 +26,8 @@ export function useUnimpededRangedNumberEntry( { max, min, onChange, value } ) { /** @type {import('../input-control/types').InputChangeCallback}*/ const changeHandler = ( next ) => { next = parseFloat( next ); - const isOverflow = next < min || next > max; - isDiverging.current = isNaN( next ) || isOverflow; - if ( isOverflow ) { + if ( next < min || next > max ) { + isDiverging.current = true; next = Math.max( min, Math.min( max, next ) ); } onChange( next ); From 2b14b0fa9cf8a253a884195b9d8bb419542a43f6 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Tue, 10 May 2022 09:30:57 -0700 Subject: [PATCH 28/28] Refine use of event existence in state syncing effect hooks --- .../components/src/input-control/input-field.tsx | 3 +-- .../components/src/input-control/reducer/reducer.ts | 12 ++++-------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/packages/components/src/input-control/input-field.tsx b/packages/components/src/input-control/input-field.tsx index 7207a337613fd..744f7f93c79dc 100644 --- a/packages/components/src/input-control/input-field.tsx +++ b/packages/components/src/input-control/input-field.tsx @@ -73,8 +73,7 @@ function InputField( value: valueProp, isPressEnterToChange, }, - onChange, - isFocused + onChange ); const { value, isDragging, isDirty } = state; diff --git a/packages/components/src/input-control/reducer/reducer.ts b/packages/components/src/input-control/reducer/reducer.ts index ff9813bc81e4c..7521a567981c1 100644 --- a/packages/components/src/input-control/reducer/reducer.ts +++ b/packages/components/src/input-control/reducer/reducer.ts @@ -139,14 +139,12 @@ function inputControlStateReducer( * @param stateReducer An external state reducer. * @param initialState The initial state for the reducer. * @param onChangeHandler A handler for the onChange event. - * @param isFocused Focus state. * @return State, dispatch, and a collection of actions. */ export function useInputControlStateReducer( stateReducer: StateReducer = initialStateReducer, initialState: Partial< InputState > = initialInputControlState, - onChangeHandler: InputChangeCallback, - isFocused: Boolean + onChangeHandler: InputChangeCallback ) { const [ state, dispatch ] = useReducer< StateReducer >( inputControlStateReducer( stateReducer ), @@ -205,9 +203,8 @@ export function useInputControlStateReducer( currentValueProp.current = initialState.value; } ); useLayoutEffect( () => { - if ( ! refEvent.current ) return; - if ( + refEvent.current && state.value !== currentValueProp.current && ! currentState.current.isDirty ) { @@ -217,16 +214,15 @@ export function useInputControlStateReducer( | PointerEvent< HTMLInputElement >, } ); } - refEvent.current = null; - }, [ state.value, state.isDragging, isFocused ] ); + }, [ state.value ] ); useLayoutEffect( () => { if ( - ! refEvent.current && initialState.value !== currentState.current.value && ! currentState.current.isDirty ) { dispatch( { value: initialState.value } ); } + if ( refEvent.current ) refEvent.current = null; }, [ initialState.value ] ); return {