-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[useAutocomplete] Pass only valid values for the getOptionLabel prop #36088
[useAutocomplete] Pass only valid values for the getOptionLabel prop #36088
Conversation
Netlify deploy previewhttps://deploy-preview-36088--material-ui.netlify.app/ Bundle size report |
85a2ee4
to
d78cb1a
Compare
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.
@rangoo94 Thanks for the PR. Can you add a test showcasing and covering the fix for the issue provided in the reproduction?
@ZeeshanTamboli, sure, not a problem - I added the unit test for regression. |
3dd9626
to
b78c1ec
Compare
packages/mui-base/src/AutocompleteUnstyled/useAutocomplete.test.js
Outdated
Show resolved
Hide resolved
const label1 = value1 === null ? '' : getOptionLabel(value1); | ||
const label2 = value2 === null ? '' : getOptionLabel(value2); |
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.
Let's not check with strict equality, in case value
is undefined
or empty string ''
using a controlled Autocomplete.
const label1 = value1 === null ? '' : getOptionLabel(value1); | |
const label2 = value2 === null ? '' : getOptionLabel(value2); | |
const label1 = !value1 ? '' : getOptionLabel(value1); | |
const label2 = !value2 ? '' : getOptionLabel(value2); |
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 will have small edge cases, i.e. false
will be treated equally to null
. I will do that though, as I believe that it will be ultimately rare, and even then the inconvenience will be low 👍
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.
In most cases, or perhaps never, value will not be false.
packages/mui-base/src/AutocompleteUnstyled/useAutocomplete.test.js
Outdated
Show resolved
Hide resolved
@ZeeshanTamboli Thanks, I have applied the expected changes. |
c84d7d8
to
4859534
Compare
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.
@rangoo94 It's a great first pull request on MUI 👌 Thank you for working on it!
The
getOptionLabel
prop shouldn't be called with fallback values, as it will not match the TypeScript's contract for genericgetOptionLabel
value.Fixes #36045
Before: https://codesandbox.io/s/angry-tree-2jcp3o?file=/src/App.tsx
After: https://codesandbox.io/s/frosty-tdd-euyttl?file=/src/App.tsx