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

Jump to bottom

[docs] Fix Select onChange call #34408

Merged
merged 1 commit into from Sep 23, 2022
Merged

[docs] Fix Select onChange call #34408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion docs/data/joy/customization/approaches/ButtonThemes.js
View file
Open in desktop
Expand Up @@ -560,7 +560,7 @@ export default function ButtonThemes() {
}}
size="sm"
value={design}
onChange={setDesign}
onChange={(event, newValue) => setDesign(newValue)}
Copy link
Member

Choose a reason for hiding this comment

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

Off-topic

Why not use?

Suggested change
onChange={(event, newValue) => setDesign(newValue)}
onChange={(event) => setDesign(event.target.value)}

so developers can more easily switch between a native <select> (for mobile) and non-native one?

Copy link
Member Author

Choose a reason for hiding this comment

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

#34158 (comment). That'd be nice, I forgot about native select. cc @michaldudak

Copy link
Member

Choose a reason for hiding this comment

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

event.target.value isn't reliable in this case. The event could be a click event on a li element that represents a select option.

Copy link
Member

Choose a reason for hiding this comment

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

It could be made reliable

material-ui/packages/mui-material/src/Select/SelectInput.js

Lines 285 to 295 in f2d6d61

// Redefine target to allow name and value to be read.
// This allows seamless integration with the most popular form libraries.
// https://github.com/mui/material-ui/issues/13485#issuecomment-676048492
// Clone the event to not override `target` of the original event.
const nativeEvent = event.nativeEvent || event;
const clonedEvent = new nativeEvent.constructor(nativeEvent.type, nativeEvent);
Object.defineProperty(clonedEvent, 'target', {
writable: true,
value: { value: newValue, name },
});

The value is of event.target.value can be: form libraries integrations.
The value of Select allowing a native select can be: using the platform where it works best.

Copy link
Member

Choose a reason for hiding this comment

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

Fair point. I'll create an issue to support event.target.value in SelectUnstyled.
As for native selects, I think it could be implemented in Material/Joy, not Base.

Copy link
Member Author

Choose a reason for hiding this comment

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

@michaldudak I think it should not block this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Very much agree, it's under the "off-topic" umbrella.

sx={{ minWidth: 160 }}
>
<Option value="github">GitHub</Option>
Expand Down
2 changes: 1 addition & 1 deletion docs/data/joy/main-features/automatic-adjustment/ListThemes.js
View file
Open in desktop
Expand Up @@ -115,7 +115,7 @@ export default function ButtonThemes() {
},
}}
value={preset}
onChange={setPreset}
onChange={(event, newValue) => setPreset(newValue)}
sx={{ minWidth: 160 }}
>
<Option value="">Default</Option>
Expand Down
2 changes: 1 addition & 1 deletion docs/src/modules/components/JoyUsageDemo.tsx
View file
Open in desktop
Expand Up @@ -438,7 +438,7 @@ export default function JoyUsageDemo<T extends { [k: string]: any } = {}>({
},
}}
value={(resolvedValue || 'none') as string}
onChange={(val) =>
onChange={(event, val) =>
setProps((latestProps) => ({
...latestProps,
[propName]: val,
Expand Down