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

[Autocomplete] Skip filtering when list of options is loading #33278

Merged
merged 3 commits into from
Sep 30, 2022
Merged

[Autocomplete] Skip filtering when list of options is loading #33278

merged 3 commits into from
Sep 30, 2022

Conversation

ndebeiss
Copy link
Contributor

@ndebeiss ndebeiss commented Jun 24, 2022

…een typed

@mui-bot
Copy link

mui-bot commented Jun 24, 2022

Details of bundle changes

Generated by 🚫 dangerJS against 3ca6c90

@michaldudak
Copy link
Member

What problem does this PR solve? It's best to create an issue first.
Also, there are no tests added that verify your change (and could help others understand what this change is about)

@michaldudak michaldudak added the component: autocomplete This is the name of the generic UI component, not the React module! label Jul 4, 2022
@ndebeiss
Copy link
Contributor Author

ndebeiss commented Jul 4, 2022

Hello
I can create an issue with the same title if you want, but it is actually a performance enhancement.
I have a list with several thousand options, with checkboxes. So the first load, when the user has not typed anything in the filter, is very long.
With the current code, each option label is generated, lower-cased, accents are removed, and it is compared to the filter to see if the option is retained.
With an empty filter, this is not necessary.
I added a test for this, but Webstorm can't run the tests, I'll look into it.

@michaldudak michaldudak changed the title when list of options is loading, no need to filter if no filter has b… [Autocomplete] Skip filtering when list of options is loading Jul 4, 2022
@michaldudak
Copy link
Member

OK, such a description in the PR is enough. I'm picky about it as in the future, when someone wants to find out when a particular line of code was changed, the PR description and comments will be the only thing to guide them. That's why it's important to have a good description of a change.

To make the CI happy, please run yarn prettier and push the changes.

@ndebeiss
Copy link
Contributor Author

ndebeiss commented Jul 5, 2022

Yes, I completely understand the need to comment on the change. I made the pull request quickly, I wasn't sure I would be able to contribute.
I did the yarn prettier and repushed.

@michaldudak
Copy link
Member

The test you've added passes even without your change, so it doesn't correctly verify the problem you're solving.

@michaldudak
Copy link
Member

Please merge or rebase on the latest master.

@ndebeiss
Copy link
Contributor Author

ndebeiss commented Sep 1, 2022

Hello
Do you need something more from me ?
regards

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Great contribution, thank you! I've updated the logic just a bit, to be more readable. The test looks good now 👍

@mnajdova mnajdova merged commit 5b7b17c into mui:master Sep 30, 2022
alexfauquette pushed a commit to alexfauquette/material-ui that referenced this pull request Oct 14, 2022
daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022
feliperli pushed a commit to jesrodri/material-ui that referenced this pull request Dec 6, 2022
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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants