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

Conversation

michaldudak
Copy link
Member

@michaldudak michaldudak commented Aug 31, 2022

The SelectUnstyled and MultiSelectUnstyled onChange callbacks did not have event as the first parameter, leading to inconsistency with other components and native HTML elements.
This PR adds the event parameter as the first one and moves the newly selected value to the second position. Because of this, it's a breaking change.

// before
<SelectUnstyled onChange={(newValue) => { /* ... */ }} />

// after
<SelectUnstyled onChange={(event, newValue) => { /* ... */ }} />

I've also changed the signature of Joy Select's onChange to match the one in Base (CC @siriwatknp).

The onHighlightChange in useListbox while not exposed by Select and MultiSelect was also updated.

Closes #33901

@michaldudak michaldudak added breaking change component: select This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base labels Aug 31, 2022
@mui-bot
Copy link

mui-bot commented Aug 31, 2022

Details of bundle changes

Generated by 🚫 dangerJS against efebd10

@siriwatknp
Copy link
Member

siriwatknp commented Sep 1, 2022

@michaldudak I can help update the Joy Select demos if you'd like.

I added @mui/joy label because it is a breaking change for Joy UI as well.

@siriwatknp siriwatknp added the package: joy-ui Specific to @mui/joy label Sep 1, 2022
@michaldudak
Copy link
Member Author

Thanks a lot, there's no need to, I can take a look at them myself.
BTW, why are there no TS demos in Joy?

@michaldudak michaldudak marked this pull request as ready for review September 1, 2022 09:30
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 9, 2022
@siriwatknp
Copy link
Member

@michaldudak Can I use 'event.target.value' for getting the selected option?

@michaldudak
Copy link
Member Author

@siriwatknp no. The event.target could be a <li> that's clicked on. It doesn't have a meaningful value. Besides, in some cases event could be null, so your safest bet would be to use the second parameter. I'd like to standardize this pattern across all input components - have an event as a first parameter and the changed value as the second one.

@siriwatknp
Copy link
Member

@siriwatknp no. The event.target could be a <li> that's clicked on. It doesn't have a meaningful value. Besides, in some cases event could be null, so your safest bet would be to use the second parameter. I'd like to standardize this pattern across all input components - have an event as a first parameter and the changed value as the second one.

👍 for leaning toward consistency (similar to the onChange in Tabs).

Signed-off-by: Siriwat K <siriwatkunaporn@gmail.com>
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 15, 2022
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me.

@michaldudak michaldudak merged commit 1350aa8 into mui:master Sep 19, 2022
@michaldudak michaldudak deleted the iss/33901-select-onchange-parameters branch September 19, 2022 08:14
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

It's great to see this change of API :D

@@ -104,7 +104,10 @@ export interface SelectOwnProps<TValue extends {}> extends SelectStaticProps {
/**
* Callback fired when an option is selected.
*/
onChange?: (value: TValue | null) => void;
onChange?: (
e: React.MouseEvent | React.KeyboardEvent | React.FocusEvent | null,
Copy link
Member

@oliviertassinari oliviertassinari Sep 19, 2022

Choose a reason for hiding this comment

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

💯 to remove these shorthands. As far as I know, these are the first in the codebase. I would argue that it's confusing for developers that are getting started, and less clear for experienced developers that are coming back to an old codebase. I think that e > event only for writing code, but if we assume that this code will be read a lot more than written, then I think it would be better with:

Suggested change
e: React.MouseEvent | React.KeyboardEvent | React.FocusEvent | null,
event: React.MouseEvent | React.KeyboardEvent | React.FocusEvent | null,

Copy link
Member Author

Choose a reason for hiding this comment

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

All right, will do a follow-up PR.

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

@oliviertassinari oliviertassinari Sep 19, 2022

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

daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022
Co-authored-by: Siriwat K <siriwatkunaporn@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: select This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base package: joy-ui Specific to @mui/joy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SelectUnstyled] Event is not passed to the onChange handler
4 participants