-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[joy-ui][Select] Support selection of multiple
options
#39454
Conversation
Netlify deploy previewBundle size reportDetails of bundle changes (Toolpad) |
|
||
const [SlotRoot, rootProps] = useSlot('root', { | ||
ref: handleRef, | ||
className: classes.root, | ||
elementType: SelectRoot, | ||
externalForwardedProps, | ||
ownerState, | ||
ownerState: ownerState as SelectOwnerState<any, boolean>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to improve this but failed 😣. TypeScript is one of the reasons I try to avoid components with union value type 🥲. It's really hard to get it right.
Thanks for working on this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel you, types in this PR tested my patience 😅
Note that in multiple selection mode, the `value` prop (and `defaultValue`) is an array. | ||
|
||
{{"demo": "SelectMultiple.js", "defaultCodeOpen": false}} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to add a couple of more demos:
#### Selected value appearance
Display avatars of selected users end with "x friends".
#### Form submission
Show what the value looks like after submitted a form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good to me. Maybe adding more demos as suggested.
d1e9cf2
to
41c2327
Compare
renderValue={(selected) => ( | ||
<Box sx={{ display: 'flex', gap: '0.25rem' }}> | ||
{selected.map((selectedOption) => ( | ||
<Chip variant="outlined" color="success"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I'll update
eef1cb2
to
db94277
Compare
db94277
to
ddac565
Compare
@siriwatknp PR is ready for review, added demos as suggested |
onSubmit={(event) => { | ||
event.preventDefault(); | ||
const formData = new FormData(event.currentTarget); | ||
const formJson = Object.fromEntries(formData.entries()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is strange that formJson.pets
is a string, not an array. Is this expected?
I got "[\"dog\"\"
instead of ["dog"]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you console.log On logging formJson.pets
it indeed prints array
, so issue you described maybe have to do something with alert
typeof formJson.pets
i'm getting type as string
. Ideally It should be object
, let me check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this expected?
Yes, this is expected because if Select
has multiple
prop then value
of hidden input
is stringified. so users need to do JSON.parse
to convert it to array
.
material-ui/packages/mui-base/src/useSelect/useSelect.ts
Lines 51 to 57 in a731e86
if (Array.isArray(selectedOption)) { | |
if (selectedOption.length === 0) { | |
return ''; | |
} | |
return JSON.stringify(selectedOption.map((o) => o.value)); | |
} |
So I've modified demo and parsed string
to array
. Now demo is looking good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.
multiple
propmultiple
options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Well done!
@siriwatknp Can this be merged ? |
multiple
optionsmultiple
options
closes: #39102
preview: https://deploy-preview-39454--material-ui.netlify.app/joy-ui/react-select/#multiple-selections