Skip to content

Commit

Permalink
Finish making InputControl et al. more controllable (#40568)
Browse files Browse the repository at this point in the history 
* Update `RangeControl` to play nice with revised `InputControl`

* Update `UnitControl` to play nice with revised `InputControl`

* Restore controlled mode to `RangeControl`

* Add missing ;

* Add comment after deleting `onChange`

* Update test of `RangeControl` to also test controlled mode

* Remove separate onChange call from reset handling in `RangeControl`

* Refine RESET logic of `InputControl` reducer

* Simplify refined RESET logic of `InputControl` reducer

* Restore initial position of `RangeControl` when without value

* Differentiate state sync effect hooks by event existence

* Add and use type `SecondaryReducer`

* Cleanup legacy `event.perist()`

* Simplify update from props in reducer

Co-authored-by: Lena Morita <lena@jaguchi.com>

* Ensure event is cleared after drag actions

* Avoid declaration of potential unused variable

* Add more reset unit tests for `RangeControl`

* Run `RangeControl` unit test in both controlled/uncontrolled modes

* Make “keep invaid values” test async

* Prevent interference of value entry in number input

* Remove unused `floatClamp` function

* Fix reset to `initialPosition`

* Fix a couple tests for controlled `RangeControl`

* Fix `RangeControl` reset

* Ensure `InputControl`’s state syncing works after focus changes

* Comment

* Ignore NaN values in `useUnimpededRangedNumberEntry`

* Refine use of event existence in state syncing effect hooks

Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
Co-authored-by: Lena Morita <lena@jaguchi.com>
  • Loading branch information
@stokesman @ciampo @mirka
3 people authored May 11, 2022
1 parent 1b5d80c commit c8a8013
Show file tree
Hide file tree
Showing 9 changed files with 180 additions and 199 deletions.
2 changes: 1 addition & 1 deletion packages/components/src/input-control/reducer/actions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export const PRESS_UP = 'PRESS_UP';
export const RESET = 'RESET';

interface EventPayload {
event?: SyntheticEvent;
event: SyntheticEvent;
}

interface Action< Type, ExtraPayload = {} > {
Expand Down
49 changes: 20 additions & 29 deletions packages/components/src/input-control/reducer/reducer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,14 @@ function inputControlStateReducer(
composedStateReducers: StateReducer
): StateReducer {
return ( state, action ) => {
// 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 ) ) {
return {
...state,
value: `${ action.value ?? '' }`,
};
}
const nextState = { ...state };

switch ( action.type ) {
Expand Down Expand Up @@ -98,7 +106,7 @@ function inputControlStateReducer(
case actions.RESET:
nextState.error = null;
nextState.isDirty = false;
nextState.value = action.payload.value || state.initialValue;
nextState.value = action.payload.value ?? state.initialValue;
break;

/**
Expand All @@ -109,10 +117,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
Expand Down Expand Up @@ -151,15 +155,7 @@ 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,
payload: { value: nextValue, event },
Expand All @@ -169,30 +165,25 @@ 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 } } );
};

const createDragEvent = ( type: actions.DragEventAction[ 'type' ] ) => (
payload: actions.DragEventAction[ 'payload' ]
) => {
refEvent.current = payload.event;
dispatch( { type, payload } );
};

/**
* 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 );

Expand All @@ -206,17 +197,19 @@ 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 >,
} );
Expand All @@ -227,11 +220,9 @@ export function useInputControlStateReducer(
initialState.value !== currentState.current.value &&
! currentState.current.isDirty
) {
reset(
initialState.value,
currentState.current._event as SyntheticEvent
);
dispatch( { value: initialState.value } );
}
if ( refEvent.current ) refEvent.current = null;
}, [ initialState.value ] );

return {
Expand Down
12 changes: 7 additions & 5 deletions packages/components/src/input-control/reducer/state.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,22 +9,24 @@ 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 type SecondaryReducer = Reducer< InputState, InputAction >;

export const initialStateReducer: StateReducer = ( state: InputState ) => state;

export const initialInputControlState: InputState = {
_event: {},
error: null,
initialValue: '',
isDirty: false,
Expand Down
103 changes: 42 additions & 61 deletions packages/components/src/range-control/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ import { useInstanceId } from '@wordpress/compose';
import BaseControl from '../base-control';
import Button from '../button';
import Icon from '../icon';
import { COLORS } from '../utils';
import { floatClamp, useControlledRangeValue } from './utils';
import { COLORS, useControlledState } from '../utils';
import { useUnimpededRangedNumberEntry } from './utils';
import InputRange from './input-range';
import RangeRail from './rail';
import SimpleTooltip from './tooltip';
Expand Down Expand Up @@ -70,13 +70,10 @@ function RangeControl(
},
ref
) {
const [ value, setValue ] = useControlledRangeValue( {
min,
max,
value: valueProp,
initial: initialPosition,
} );
const isResetPendent = useRef( false );
const [ value, setValue ] = useControlledState( valueProp, {
fallback: null,
} );

if ( step === 'any' ) {
// The tooltip and number input field are hidden when the step is "any"
Expand All @@ -102,15 +99,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 );

Expand All @@ -129,23 +126,20 @@ function RangeControl(
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 );
const someNumberInputProps = useUnimpededRangedNumberEntry( {
max,
min,
value: usedValue ?? '',
onChange: ( nextValue ) => {
if ( ! isNaN( nextValue ) ) {
setValue( nextValue );
onChange( nextValue );
isResetPendent.current = false;
} else if ( allowReset ) {
isResetPendent.current = true;
}
onChange( nextValue );
isResetPendent.current = false;
} else if ( allowReset ) {
isResetPendent.current = true;
}
};
},
} );

const handleOnInputNumberBlur = () => {
if ( isResetPendent.current ) {
Expand All @@ -155,30 +149,20 @@ function RangeControl(
};

const handleOnReset = () => {
let resetValue = parseFloat( resetFallbackValue );
let onChangeResetValue = resetValue;
const resetValue = parseFloat( resetFallbackValue );

if ( isNaN( resetValue ) ) {
resetValue = null;
onChangeResetValue = undefined;
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 );

/**
* 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 <input /> 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 );
Expand All @@ -197,7 +181,7 @@ function RangeControl(
};

const offsetStyle = {
[ isRTL() ? 'right' : 'left' ]: fillValueOffset,
[ isRTL() ? 'right' : 'left' ]: fillPercent,
};

return (
Expand Down Expand Up @@ -235,7 +219,7 @@ function RangeControl(
onMouseLeave={ onMouseLeave }
ref={ setRef }
step={ step }
value={ inputSliderValue }
value={ usedValue ?? '' }
/>
<RangeRail
aria-hidden={ true }
Expand All @@ -245,13 +229,13 @@ function RangeControl(
min={ min }
railColor={ railColor }
step={ step }
value={ rangeFillValue }
value={ usedValue }
/>
<Track
aria-hidden={ true }
className="components-range-control__track"
disabled={ disabled }
style={ { width: fillValueOffset } }
style={ { width: fillPercent } }
trackColor={ trackColor }
/>
<ThumbWrapper style={ offsetStyle } disabled={ disabled }>
Expand Down Expand Up @@ -285,13 +269,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 && (
Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/range-control/rail.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export default function RangeRail( {
min = 0,
max = 100,
step = 1,
value = 0,
value,
...restProps
} ) {
return (
Expand All @@ -29,7 +29,7 @@ export default function RangeRail( {
min={ min }
max={ max }
step={ step }
value={ value }
value={ value ?? ( max - min ) / 2 + min }
/>
) }
</>
Expand Down
Loading

0 comments on commit c8a8013

Please sign in to comment.