Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

InputControl: Fix undo when changing padding values #40518

Merged
merged 11 commits into from
May 30, 2022
Merged

InputControl: Fix undo when changing padding values #40518

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
070a66e
Fix undo when changing padding values
youknowriad Apr 21, 2022
769e69d
Finish making `InputControl` et al. more controllable (#40568)
stokesman May 11, 2022
bbdbfc8
Fix `RangeControl` reset from values out-of-range by multiple digits
stokesman May 17, 2022
80e7d58
Add changelog entry
stokesman May 17, 2022
f1f4daa
Revert "Fix `RangeControl` reset from values out-of-range by multiple…
stokesman May 29, 2022
0de33ab
Revert "Finish making `InputControl` et al. more controllable (#40568)"
stokesman May 29, 2022
8952d45
Have `InputControl` favor entered value for a render cycle
stokesman May 26, 2022
32eb218
Run propagation effect when `isDirty` changes
stokesman May 28, 2022
bb6b932
Use event existence to eliminate extraneous `onChange` calls
stokesman May 28, 2022
d79aa03
Keep change handler in a ref to pass `react-hooks/exhaustive-deps`
stokesman May 28, 2022
3ac6020
Cleanup legacy `event.persist` calls
stokesman May 28, 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
18 changes: 10 additions & 8 deletions packages/components/src/input-control/reducer/reducer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,7 @@ function inputControlStateReducer(
break;
}

if ( action.payload.event ) {
nextState._event = action.payload.event;
}
nextState._event = action.payload.event;

/**
* Send the nextState + action to the composedReducers via
Expand Down Expand Up @@ -211,7 +209,11 @@ export function useInputControlStateReducer(
currentValueProp.current = initialState.value;
} );
useLayoutEffect( () => {
if ( state.value !== currentValueProp.current && ! state.isDirty ) {
if (
currentState.current._event !== undefined &&
state.value !== currentValueProp.current &&
! state.isDirty
) {
onChangeHandler( state.value ?? '', {
event: currentState.current._event as
| ChangeEvent< HTMLInputElement >
Expand All @@ -224,10 +226,10 @@ export function useInputControlStateReducer(
initialState.value !== currentState.current.value &&
! currentState.current.isDirty
) {
reset(
initialState.value,
currentState.current._event as SyntheticEvent
);
dispatch( {
type: actions.RESET,
payload: { value: initialState.value },
} );
Copy link
Contributor

Choose a reason for hiding this comment

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

Two reasons for switching to dispatch:

  • reset is typed to require passing an event for the second argument and I didn't want to change the typing because in most cases it would be important to pass the event.
  • The exhaustive-deps lint rule knows dispatch is stable and won't complain when it's not in the dependencies.

My remaining peeve with this is I think it'd be better to not reuse RESET here and maybe reintroduce UPDATE. It's only a theoretical concern only and for now a non-issue. The concern would be that someday a component wants to specialize RESET in a way that should not happen every time the value prop has changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation of dispatch over reset, it makes sense. I also agree regarding RESET vs UPDATE and that it is a potential future concern but that can be addressed down the line and separately to this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds like a good point, if we were to keep the internal reducer!

At the same time, after this fix is merged I'd like to explore ways to simplify InputControl (and consequently its derived components), including the removal of the internal reducer (see this comment for more details)

}
}, [ initialState.value ] );

Expand Down
5 changes: 2 additions & 3 deletions packages/components/src/input-control/reducer/state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
/**
* External dependencies
*/
import type { Reducer } from 'react';
import type { Reducer, SyntheticEvent } from 'react';

/**
* Internal dependencies
*/
import type { InputAction } from './actions';

export interface InputState {
_event: Event | {};
_event?: SyntheticEvent;
error: unknown;
initialValue?: string;
isDirty: boolean;
Expand All @@ -24,7 +24,6 @@ export type StateReducer = Reducer< InputState, InputAction >;
export const initialStateReducer: StateReducer = ( state: InputState ) => state;

export const initialInputControlState: InputState = {
_event: {},
error: null,
initialValue: '',
isDirty: false,
Expand Down