-
-
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
[material-ui][Autocomplete] Fix behavior when there are duplicate labels #36426
[material-ui][Autocomplete] Fix behavior when there are duplicate labels #36426
Conversation
Related to #26492 |
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.
Index should not be used for a key in React in case the options are reordered, added or deleted.
Let us provide the "key" parameter ourselves? 🙏 We use icons to distinguish between options with the same name |
Yes, this would make sense. |
Netlify deploy previewhttps://deploy-preview-36426--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
…completeDuplicateLabel
I have added the getOptionKey API to allow for specifying a key. |
Co-authored-by: Zeeshan Tamboli <[email protected]> Signed-off-by: SHIMA RYUHEI <[email protected]>
Co-authored-by: Zeeshan Tamboli <[email protected]> Signed-off-by: SHIMA RYUHEI <[email protected]>
Co-authored-by: Zeeshan Tamboli <[email protected]> Signed-off-by: SHIMA RYUHEI <[email protected]>
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.
Apologies. After re-reading the discussion in #26492, I noticed that the getOptionKey
prop was previously added (#32910), but later reverted in #33142. The reasoning behind this decision was explained in #26492 (comment).
However, I believe that including index with getOptionLabel is not the appropriate solution, as mentioned in #36426 (review). I'd go with having a getOptionKey
prop.
cc @oliviertassinari @mui/core
I wasn't against |
@michaldudak @oliviertassinari Any update regarding this PR? We implemented our own |
Ok, it seems that we can't use #24198 (comment) because it would lead to extra DOM operations, and regressing on #24213 that requires to keep the same DOM element. I can think of 3 different solutions:
So, it seems that option 3 wins. If we push with this new prop, I think that the main challenge will be for developers to figure out how to solve their issue. They would get a key conflict but no pointers on how to solve it. I guess that if this comes frequently enough (for now, we don't know), we could create a dedicated warning for it. |
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.
@islandryu Thanks for your contribution.
I have made some changes after confirmation to go with this route in #36426 (comment). It seems fine to me.
I'd like @michaldudak to review this.
but we need a string for the key.
@oliviertassinari Why do we specifically need a string for the key? Why can't it be anything?
It looks OK in general. I had one doc-related comment, but besides that, I think it's good to go. |
…els (mui#36426) Signed-off-by: SHIMA RYUHEI <[email protected]> Co-authored-by: Zeeshan Tamboli <[email protected]>
…els (mui#36426) Signed-off-by: SHIMA RYUHEI <[email protected]> Co-authored-by: Zeeshan Tamboli <[email protected]>
…els (mui#36426) Signed-off-by: SHIMA RYUHEI <[email protected]> Co-authored-by: Zeeshan Tamboli <[email protected]>
I'm glad it's solved, it was really big problem for me! Many thanks to Mr Contributor 🫡 |
…els (mui#36426) Signed-off-by: SHIMA RYUHEI <[email protected]> Co-authored-by: Zeeshan Tamboli <[email protected]>
…els (mui#36426) Signed-off-by: SHIMA RYUHEI <[email protected]> Co-authored-by: Zeeshan Tamboli <[email protected]>
…els (mui#36426) Signed-off-by: SHIMA RYUHEI <[email protected]> Co-authored-by: Zeeshan Tamboli <[email protected]>
Fixed a behavior that caused label refinement to go wrong when two labels were the same.
code
Before modification
localhost_3000_playground_.-.Google.Chrome.2023-03-05.07-27-43.mp4
After modification
localhost_3000_playground_.-.Google.Chrome.2023-03-05.07-29-04.mp4
Fixes #26492