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

[material-ui][Autocomplete] check for previousProps value being not-null after handling option click #42644

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

giulio-opal
Copy link

We were using an old beta version (@material-ui/lab) of Autocomplete in our codebase and after finally upgrading to v5 we noticed a regression in one of our components based on Autocomplete due to this line checking for previousProps.value.length while previousProps.value was null. I think this is the only place were previousProps.value.length is checked.

Context:

We probably do some weird hack in which we switch from a non-multiple to multiple autocomplete dropdown, this is because on first render, we populate the options with a list and then upon user selection, load another list and allow the user to select multiple values out of the dropdown.

Please let me know if there's any test I should be adding/modifying. Thank you!

@zannager zannager added the component: autocomplete This is the name of the generic UI component, not the React module! label Jun 14, 2024
@zannager zannager requested a review from sai6855 June 14, 2024 13:38
Copy link
Contributor

@sai6855 sai6855 left a comment

Choose a reason for hiding this comment

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

@giulio-opal can you provide sandbox which reproduces error you are describing?

@ZeeshanTamboli ZeeshanTamboli added the package: material-ui Specific to @mui/material label Jun 19, 2024
@giulio-opal
Copy link
Author

@giulio-opal can you provide sandbox which reproduces error you are describing?

Sorry for the slow turnaround, here's a codesandbox: https://codesandbox.io/p/sandbox/nervous-bush-lzzmfw
In short, if the data comes from a remote source, eg. through a query (in this case I've implemented it with timeout hooks), switching from single to multiple triggers this issue. This is a simplified version of our dropdown.

@giulio-opal giulio-opal requested a review from sai6855 June 21, 2024 17:52
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

We probably do some weird hack ...

So you shouldn't do this in your app in the first place?

@ZeeshanTamboli ZeeshanTamboli changed the title [material-ui][autocomplete] check for previousProps value being not-null after handling option click [material-ui][Autocomplete] check for previousProps value being not-null after handling option click Jun 30, 2024
@giulio-opal
Copy link
Author

We probably do some weird hack ...

So you shouldn't do this in your app in the first place?

This null check is very hard to patch from our side, and our use of Autocomplete is very similar to the example provided, which should be straightforward.
There's no check in place for a nullable value for the reference to previousProps.

(The "weird hack" was mentioned because I hadn't isolated some additional tweaks that we do for UX purposes on Autocomplete, which then I did in the sandbox)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: autocomplete This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants