-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[Autocomplete] Better synchronize the highlight with the value #19923
Conversation
Details of bundle changes.Comparing: 33a7c26...a33ae0f
|
@jonatanklosko Thanks for the test. |
93405f6
to
a33ae0f
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.
I must say, the logic we are changing here is complex, I have rarely seen this in the repository. I have tried to update the tests to better cover the issues. I have also merged two effects.
The only part I'm not very happy about is that we ignore 3 dependencies in the effect: options
, getOptionSelected
, getOptionLabel
but I think that it's a tradeoff for the best. I would like to avoid a case where the users rerender at each keystroke, leading to a stuck highlight. Maybe we can reconsider it later. At least we have these two great test cases.
@captain-yossarian If you could double-check the changes, it would be highly appreciated, I'm not very confident. |
@oliviertassinari I will thoroughly check it and let you know P.S. I did not know about #19705 issue |
@oliviertassinari I have tested #19779 and #19637 and it works as expected. |
@captain-yossarian Thanks |
Closes #19779
Closes #19637
Closes #19705