-
-
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
[material-ui][Autocomplete] Fix regression in freeSolo Autocomplete to render custom Popper/Paper when options are empty #44266
Conversation
Netlify deploy previewhttps://deploy-preview-44266--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
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.
Behavior wise looks good. I asked @mnajdova to do a second review.
This looks too much like we are trying to fix the issue the specific user had. We should either always render the popup when there are no options, or never render it. It shouldn't depend on whether the user provided a custom paper. I may have a custom paper but I don't want it to appear if there are no options. It almost looks like we should provide a prop for this, in case people truly need these two different use-cases. It strange that our reaction to the issues is:
If there is a use-case where people want to have the thing render regardless of whether the options array is empty, to me this is a new feature and we should have a prop to configure this. If we solved it that way the first time, we wouldn't have had regression, as we could have set up the prop default value to be aligned with the behavior we had before. |
You're right. Initially, #40843 looked like a bug since custom styles on the closed Popper were showing up like a border. I think the best and logical approach is to not render the Popper if there are no options. But now I'm unsure how to proceed with #44048.
Good point. I missed this scenario. |
as the person who brought up the original issue, i agree that this may not be the ideal fix for the regression as it adds a new conditional for displaying the poppper in the renderer rather than rectifying the internal state in the autocomplete hook which, as i mentioned in my original bug report, is currently incorrect. as it is now with this regression in the code if the popper is not being displayed because of no items being present the autocomplete hook's open state will still "think" the popper is open even though it's not being displayed.
i could be onboard with a new prop if it gets us what we had before. the only problem i have with that is technically this regression was a breaking change of behavior in the 5.x release and if you're adding the functionality back as a new feature i imagine it would only show up in 6.x. if that were the case, it seems like a big ask to ask users that want the behavior we already had in 5.x (before the regression) to upgrade to the latest major version to get that behavior back. i think the ideal fix is refactoring the fix in #41300 to not change the renderer output manually but instead fix the autocomplete hook's internal state of "open" to correctly take the option count into consideration. to resolve the regression it should also consider custom paper/poppers but how it decides to consider them could be based on a prop, as @mnajdova suggested. in the 5.x release i think it'd make sense for it to default to allowing the autocomplete to be "open" if there is a custom popper with no options as that was the behavior that existed before the regression. in 6.x i think you should be free to do whatever you deem appropriate as that is when breaking changes should be introduced, but defaulting to not displaying with no options but a prop allowing the custom paper to be displayed is acceptable i think. |
@mnajdova @aarongarciah Are there plans for a v5 release? I can revise the #41300 fix for v5 and, for v6, fix the open state and add the new prop. Edit: I'm also unsure if adding another prop to Material UI Autocomplete is ideal, given the already large number of props. |
My recommendation would be:
This should solve both issues without disrupting people with introducing breaking changes. |
Fixes #44048
Regression from #41300
Ensure custom Popper and Paper components render in freeSolo mode and when there are no options.
We also maintain the fix from #41300.
Before fix: https://codesandbox.io/p/sandbox/material-ui-cra-ts-forked-4nxwj7
After fix: https://codesandbox.io/p/sandbox/material-ui-cra-ts-forked-qgg2rr