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

[Select][base] Add event parameter to the onChange callback #34158

Merged
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -165,7 +165,7 @@ export default function UnstyledSelectsMultiple() {
const [value, setValue] = React.useState(10);
return (
<div>
<CustomSelect value={value} onChange={setValue}>
<CustomSelect value={value} onChange={(e, newValue) => setValue(newValue)}>
<StyledOption value={10}>Ten</StyledOption>
<StyledOption value={20}>Twenty</StyledOption>
<StyledOption value={30}>Thirty</StyledOption>
Expand Down
Expand Up @@ -154,7 +154,7 @@ export default function UnstyledSelectsMultiple() {
const [value, setValue] = React.useState<number | null>(10);
return (
<div>
<CustomSelect value={value} onChange={setValue}>
<CustomSelect value={value} onChange={(e, newValue) => setValue(newValue)}>
Copy link
Member

Choose a reason for hiding this comment

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

based on the comment above, I think that it either:

Suggested change
<CustomSelect value={value} onChange={(e, newValue) => setValue(newValue)}>
<CustomSelect value={value} onChange={(event, newValue) => setValue(newValue)}>

or

Suggested change
<CustomSelect value={value} onChange={(e, newValue) => setValue(newValue)}>
<CustomSelect value={value} onChange={(_, newValue) => setValue(newValue)}>

<StyledOption value={10}>Ten</StyledOption>
<StyledOption value={20}>Twenty</StyledOption>
<StyledOption value={30}>Thirty</StyledOption>
Expand Down
@@ -1,4 +1,4 @@
<CustomSelect value={value} onChange={setValue}>
<CustomSelect value={value} onChange={(e, newValue) => setValue(newValue)}>
<StyledOption value={10}>Ten</StyledOption>
<StyledOption value={20}>Twenty</StyledOption>
<StyledOption value={30}>Thirty</StyledOption>
Expand Down
Expand Up @@ -187,7 +187,10 @@ export default function UnstyledSelectObjectValues() {
const [character, setCharacter] = React.useState(characters[0]);
return (
<div>
<CustomSelect value={character} onChange={setCharacter}>
<CustomSelect
value={character}
onChange={(e, newValue) => setCharacter(newValue)}
>
{characters.map((c) => (
<StyledOption key={c.name} value={c}>
{c.name}
Expand Down
Expand Up @@ -181,7 +181,10 @@ export default function UnstyledSelectObjectValues() {
const [character, setCharacter] = React.useState<Character | null>(characters[0]);
return (
<div>
<CustomSelect value={character} onChange={setCharacter}>
<CustomSelect
value={character}
onChange={(e, newValue) => setCharacter(newValue)}
>
{characters.map((c) => (
<StyledOption key={c.name} value={c}>
{c.name}
Expand Down
@@ -1,4 +1,7 @@
<CustomSelect value={character} onChange={setCharacter}>
<CustomSelect
value={character}
onChange={(e, newValue) => setCharacter(newValue)}
>
{characters.map((c) => (
<StyledOption key={c.name} value={c}>
{c.name}
Expand Down
Expand Up @@ -204,7 +204,11 @@ export default function UnstyledSelectObjectValues() {
<div>
<Header>Default behavior</Header>
<form onSubmit={handleSubmit}>
<CustomSelect value={character} onChange={setCharacter} name="character">
<CustomSelect
value={character}
onChange={(e, newValue) => setCharacter(newValue)}
name="character"
>
{characters.map((c) => (
<StyledOption key={c.name} value={c}>
{c.name}
Expand All @@ -219,7 +223,7 @@ export default function UnstyledSelectObjectValues() {
<form onSubmit={handleSubmit}>
<CustomSelect
value={character}
onChange={setCharacter}
onChange={(e, newValue) => setCharacter(newValue)}
getSerializedValue={getSerializedValue}
name="character"
>
Expand Down
Expand Up @@ -199,7 +199,11 @@ export default function UnstyledSelectObjectValues() {
<div>
<Header>Default behavior</Header>
<form onSubmit={handleSubmit}>
<CustomSelect value={character} onChange={setCharacter} name="character">
<CustomSelect
value={character}
onChange={(e, newValue) => setCharacter(newValue)}
name="character"
>
{characters.map((c) => (
<StyledOption key={c.name} value={c}>
{c.name}
Expand All @@ -214,7 +218,7 @@ export default function UnstyledSelectObjectValues() {
<form onSubmit={handleSubmit}>
<CustomSelect
value={character}
onChange={setCharacter}
onChange={(e, newValue) => setCharacter(newValue)}
getSerializedValue={getSerializedValue}
name="character"
>
Expand Down
2 changes: 1 addition & 1 deletion docs/data/joy/components/select/SelectClearable.js
Expand Up @@ -12,7 +12,7 @@ export default function SelectBasic() {
action={action}
value={value}
placeholder="Favorite pet…"
onChange={setValue}
onChange={(e, newValue) => setValue(newValue)}
{...(value && {
// display the button and remove select indicator
// when user has selected a value
Expand Down
6 changes: 5 additions & 1 deletion docs/data/joy/components/select/SelectFieldDemo.js
Expand Up @@ -15,7 +15,11 @@ export default function SelectFieldDemo() {
>
Favorite pet
</FormLabel>
<Select defaultValue="dog" value={value} onChange={setValue}>
<Select
defaultValue="dog"
value={value}
onChange={(e, newValue) => setValue(newValue)}
>
<Option value="dog">Dog</Option>
<Option value="cat">Cat</Option>
<Option value="fish">Fish</Option>
Expand Down
2 changes: 1 addition & 1 deletion docs/data/joy/components/select/SelectUsage.js
Expand Up @@ -49,7 +49,7 @@ export default function SelectUsage() {
{...props}
action={action}
value={value}
onChange={setValue}
onChange={(e, newValue) => setValue(newValue)}
{...(value && {
endDecorator: (
<IconButton
Expand Down
@@ -1,3 +1,4 @@
import * as React from 'react';
import { expect } from 'chai';
import { ActionTypes, ListboxAction, ListboxState } from './useListbox.types';
import defaultReducer from './defaultListboxReducer';
Expand All @@ -13,6 +14,7 @@ describe('useListbox defaultReducer', () => {
const action: ListboxAction<string> = {
type: ActionTypes.setValue,
value: 'foo',
event: null,
};
const result = defaultReducer(state, action);
expect(result.selectedValue).to.equal('foo');
Expand Down Expand Up @@ -327,6 +329,7 @@ describe('useListbox defaultReducer', () => {
const action: ListboxAction<string> = {
type: ActionTypes.textNavigation,
searchString: 'th',
event: {} as React.KeyboardEvent,
props: {
options: ['one', 'two', 'three', 'four', 'five'],
disableListWrap: false,
Expand All @@ -351,6 +354,7 @@ describe('useListbox defaultReducer', () => {
const action: ListboxAction<string> = {
type: ActionTypes.textNavigation,
searchString: 'z',
event: {} as React.KeyboardEvent,
props: {
options: ['one', 'two', 'three', 'four', 'five'],
disableListWrap: false,
Expand All @@ -375,6 +379,7 @@ describe('useListbox defaultReducer', () => {
const action: ListboxAction<string> = {
type: ActionTypes.textNavigation,
searchString: 't',
event: {} as React.KeyboardEvent,
props: {
options: ['one', 'two', 'three', 'four', 'five'],
disableListWrap: false,
Expand All @@ -399,6 +404,7 @@ describe('useListbox defaultReducer', () => {
const action: ListboxAction<string> = {
type: ActionTypes.textNavigation,
searchString: 't',
event: {} as React.KeyboardEvent,
props: {
options: ['one', 'two', 'three', 'four', 'five'],
disableListWrap: false,
Expand All @@ -423,6 +429,7 @@ describe('useListbox defaultReducer', () => {
const action: ListboxAction<string> = {
type: ActionTypes.textNavigation,
searchString: 'one',
event: {} as React.KeyboardEvent,
props: {
options: ['one', 'two', 'three', 'four', 'five'],
disableListWrap: true,
Expand Down
Expand Up @@ -20,7 +20,7 @@ describe('useControllableReducer', () => {
return state;
});

const actionToDispatch = { type: ActionTypes.setValue as const, value: 'b' };
const actionToDispatch = { type: ActionTypes.setValue as const, value: 'b', event: null };
const TestComponent = () => {
const props: UseListboxPropsWithDefaults<string> = {
options: ['a', 'b', 'c'],
Expand Down Expand Up @@ -52,7 +52,7 @@ describe('useControllableReducer', () => {
return state;
});

const actionToDispatch = { type: ActionTypes.setValue as const, value: 'b' };
const actionToDispatch = { type: ActionTypes.setValue as const, value: 'b', event: null };
const TestComponent = () => {
const props: UseListboxPropsWithDefaults<string> = {
options: ['a', 'b', 'c'],
Expand Down Expand Up @@ -82,7 +82,7 @@ describe('useControllableReducer', () => {
};
});

const actionToDispatch = { type: ActionTypes.setValue as const, value: 'b' };
const actionToDispatch = { type: ActionTypes.setValue as const, value: 'b', event: null };
const handleChange = spy();
const handleHighlightChange = spy();

Expand All @@ -105,7 +105,7 @@ describe('useControllableReducer', () => {
};

render(<TestComponent />);
expect(handleChange.getCalls()[0].args[0]).to.equal('b');
expect(handleChange.getCalls()[0].args[1]).to.equal('b');
expect(handleHighlightChange.notCalled).to.equal(true);
});

Expand All @@ -117,7 +117,11 @@ describe('useControllableReducer', () => {
};
});

const actionToDispatch = { type: ActionTypes.setHighlight as const, highlight: 'b' };
const actionToDispatch = {
type: ActionTypes.setHighlight as const,
highlight: 'b',
event: null,
};
const handleChange = spy();
const handleHighlightChange = spy();

Expand All @@ -140,7 +144,7 @@ describe('useControllableReducer', () => {
};

render(<TestComponent />);
expect(handleHighlightChange.getCalls()[0].args[0]).to.equal('b');
expect(handleHighlightChange.getCalls()[0].args[1]).to.equal('b');
expect(handleChange.notCalled).to.equal(true);
});
});
Expand Down
56 changes: 32 additions & 24 deletions packages/mui-base/src/ListboxUnstyled/useControllableReducer.ts
Expand Up @@ -45,42 +45,44 @@ function useStateChangeDetection<TOption>(
nextState: ListboxState<TOption>,
internalPreviousState: ListboxState<TOption>,
propsRef: React.RefObject<UseListboxPropsWithDefaults<TOption>>,
hasDispatchedActionRef: React.MutableRefObject<boolean>,
lastActionRef: React.MutableRefObject<ListboxAction<TOption> | null>,
) {
React.useEffect(() => {
if (!propsRef.current || !hasDispatchedActionRef.current) {
if (!propsRef.current || lastActionRef.current === null) {
// Detect changes only if an action has been dispatched.
return;
}

hasDispatchedActionRef.current = false;

const previousState = getControlledState(internalPreviousState, propsRef.current);
const { multiple, optionComparer } = propsRef.current;

if (multiple) {
const previousSelectedValues = (previousState?.selectedValue ?? []) as TOption[];
const nextSelectedValues = nextState.selectedValue as TOption[];
const onChange = propsRef.current.onChange as ((value: TOption[]) => void) | undefined;
const onChange = propsRef.current.onChange as
| ((
e: React.MouseEvent | React.KeyboardEvent | React.FocusEvent | null,
value: TOption[],
) => void)
| undefined;

if (!areArraysEqual(nextSelectedValues, previousSelectedValues, optionComparer)) {
onChange?.(nextSelectedValues);
onChange?.(lastActionRef.current.event, nextSelectedValues);
}
} else {
const previousSelectedValue = previousState?.selectedValue as TOption | null;
const nextSelectedValue = nextState.selectedValue as TOption | null;
const onChange = propsRef.current.onChange as ((value: TOption | null) => void) | undefined;
const onChange = propsRef.current.onChange as
| ((
e: React.MouseEvent | React.KeyboardEvent | React.FocusEvent | null,
value: TOption | null,
) => void)
| undefined;

if (!areOptionsEqual(nextSelectedValue, previousSelectedValue, optionComparer)) {
onChange?.(nextSelectedValue);
onChange?.(lastActionRef.current.event, nextSelectedValue);
}
}
}, [nextState.selectedValue, internalPreviousState, propsRef, hasDispatchedActionRef]);

React.useEffect(() => {
if (!propsRef.current) {
return;
}

// Fires the highlightChange event when reducer returns changed `highlightedValue`.
if (
Expand All @@ -90,9 +92,20 @@ function useStateChangeDetection<TOption>(
propsRef.current.optionComparer,
)
) {
propsRef.current?.onHighlightChange?.(nextState.highlightedValue);
propsRef.current?.onHighlightChange?.(
lastActionRef.current.event,
nextState.highlightedValue,
);
}
}, [nextState.highlightedValue, internalPreviousState.highlightedValue, propsRef]);

lastActionRef.current = null;
}, [
nextState.selectedValue,
nextState.highlightedValue,
internalPreviousState,
propsRef,
lastActionRef,
]);
}

export default function useControllableReducer<TOption>(
Expand All @@ -105,7 +118,7 @@ export default function useControllableReducer<TOption>(
const propsRef = React.useRef(props);
propsRef.current = props;

const hasDispatchedActionRef = React.useRef(false);
const actionRef = React.useRef<ListboxAction<TOption> | null>(null);

const initialSelectedValue =
(value === undefined ? defaultValue : value) ?? (props.multiple ? [] : null);
Expand All @@ -117,7 +130,7 @@ export default function useControllableReducer<TOption>(

const combinedReducer = React.useCallback(
(state: ListboxState<TOption>, action: ListboxAction<TOption>) => {
hasDispatchedActionRef.current = true;
actionRef.current = action;

if (externalReducer) {
return externalReducer(getControlledState(state, propsRef.current), action);
Expand All @@ -135,11 +148,6 @@ export default function useControllableReducer<TOption>(
previousState.current = nextState;
}, [previousState, nextState]);

useStateChangeDetection<TOption>(
nextState,
previousState.current,
propsRef,
hasDispatchedActionRef,
);
useStateChangeDetection<TOption>(nextState, previousState.current, propsRef, actionRef);
return [getControlledState(nextState, propsRef.current), dispatch];
}
4 changes: 4 additions & 0 deletions packages/mui-base/src/ListboxUnstyled/useListbox.ts
Expand Up @@ -86,6 +86,7 @@ export default function useListbox<TOption>(props: UseListboxParameters<TOption>

dispatch({
type: ActionTypes.optionsChange,
event: null,
options,
previousOptions: previousOptions.current,
props: propsWithDefaults,
Expand All @@ -101,6 +102,7 @@ export default function useListbox<TOption>(props: UseListboxParameters<TOption>
(option: TOption | TOption[] | null) => {
dispatch({
type: ActionTypes.setValue,
event: null,
value: option,
});
},
Expand All @@ -111,6 +113,7 @@ export default function useListbox<TOption>(props: UseListboxParameters<TOption>
(option: TOption | null) => {
dispatch({
type: ActionTypes.setHighlight,
event: null,
highlight: option,
});
},
Expand Down Expand Up @@ -203,6 +206,7 @@ export default function useListbox<TOption>(props: UseListboxParameters<TOption>

dispatch({
type: ActionTypes.textNavigation,
event,
searchString: textCriteria.searchString,
props: propsWithDefaults,
});
Expand Down