Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Finish making InputControl et al. more controllable #40568

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
f943b5b
Update `RangeControl` to play nice with revised `InputControl`
stokesman Apr 23, 2022
e662bde
Update `UnitControl` to play nice with revised `InputControl`
stokesman Apr 23, 2022
0077735
Restore controlled mode to `RangeControl`
stokesman Apr 25, 2022
295035c
Add missing ;
ciampo Apr 25, 2022
739372f
Add comment after deleting `onChange`
ciampo Apr 25, 2022
cd1b4df
Update test of `RangeControl` to also test controlled mode
stokesman Apr 25, 2022
9a3804f
Remove separate onChange call from reset handling in `RangeControl`
stokesman Apr 25, 2022
4dc4674
Refine RESET logic of `InputControl` reducer
stokesman Apr 25, 2022
302096d
Simplify refined RESET logic of `InputControl` reducer
stokesman Apr 26, 2022
f3c881f
Restore initial position of `RangeControl` when without value
stokesman Apr 26, 2022
19fd100
Differentiate state sync effect hooks by event existence
stokesman Apr 26, 2022
68a02ea
Add and use type `SecondaryReducer`
stokesman Apr 26, 2022
d6b43bb
Cleanup legacy `event.perist()`
stokesman Apr 26, 2022
b3e9d88
Simplify update from props in reducer
stokesman Apr 26, 2022
547a7c7
Ensure event is cleared after drag actions
stokesman Apr 26, 2022
89affab
Avoid declaration of potential unused variable
stokesman Apr 26, 2022
06d16fd
Add more reset unit tests for `RangeControl`
stokesman Apr 28, 2022
ce0ac42
Run `RangeControl` unit test in both controlled/uncontrolled modes
stokesman Apr 28, 2022
3800d28
Make “keep invaid values” test async
stokesman May 2, 2022
ff6e10a
Prevent interference of value entry in number input
stokesman May 2, 2022
e427a0e
Remove unused `floatClamp` function
stokesman May 2, 2022
38c0158
Fix reset to `initialPosition`
stokesman May 2, 2022
8930905
Fix a couple tests for controlled `RangeControl`
stokesman May 2, 2022
cb00749
Fix `RangeControl` reset
stokesman May 2, 2022
fa2723d
Ensure `InputControl`’s state syncing works after focus changes
stokesman May 2, 2022
085fd03
Comment
stokesman May 10, 2022
29634d7
Ignore NaN values in `useUnimpededRangedNumberEntry`
stokesman May 10, 2022
2b14b0f
Refine use of event existence in state syncing effect hooks
stokesman May 10, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/components/src/input-control/reducer/actions.ts
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
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

When does this happen (action without type)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be in what I'm calling the intake effect

useLayoutEffect( () => {
if (
initialState.value !== currentState.current.value &&
! currentState.current.isDirty
) {
dispatch( { value: initialState.value } );
}
if ( refEvent.current ) refEvent.current = null;
}, [ initialState.value ] );

That was dispatching via reset in the base PR but in looking for a way to distinguish the update from props I landed on making the bare dispatch with no action type. I'd considered implementing another action (or bringing back update that used to exist and was used only for this purpose (props coming in)) but I went with the bare dispatch because I thought it would make for a little less changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd have just used another action but it's not important :)

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