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] Improve autoSelect logic #19270

Closed
1 task done
castroCrea opened this issue Jan 17, 2020 · 20 comments · Fixed by #19384
Closed
1 task done

[Autocomplete] Improve autoSelect logic #19270

castroCrea opened this issue Jan 17, 2020 · 20 comments · Fixed by #19384
Labels
bug 🐛 Something doesn't work component: autocomplete This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@castroCrea
Copy link

For AutoComplete, on Blur when multiple and freeSolo is set.

It will be nice that the value of the input field, validate, like when we hit Enter.

  • I have searched the issues of this repository and believe that this is not a duplicate.

Summary 💡

On multiple and freeSolo, on blur the value in the input is automatically validated

Motivation 🔦

When you have multiple input and one is auto complete, the user expected that when he leave the filed the value is taking care of.

@oliviertassinari
Copy link
Member

@castroCrea I don't understand, could you provide more details?

@oliviertassinari oliviertassinari added status: waiting for author Issue with insufficient information component: autocomplete This is the name of the generic UI component, not the React module! labels Jan 17, 2020
@castroCrea
Copy link
Author

I'm sorry for not been clear.

For example go on https://material-ui.com/components/autocomplete/#multiple-values the freeSolo one,
enter a freeSolo value without hit Enter, then hit Tab
The value is not Saved, it doesn't show on onChange props.
The value of the input field is lost if in spite of Enter you hit Tab.

Me I use it, for email autocompletion, in email form, the user often wrote an email and hit tab to move on.

Is that more understandable ?

@oliviertassinari oliviertassinari added support: Stack Overflow Please ask the community on Stack Overflow and removed status: waiting for author Issue with insufficient information labels Jan 17, 2020
@support
Copy link

support bot commented Jan 17, 2020

👋 Thanks for using Material-UI!

We use GitHub issues exclusively as a bug and feature requests tracker, however,
this issue appears to be a support request.

For support, please check out https://material-ui.com/getting-started/support/. Thanks!

If you have a question on StackOverflow, you are welcome to link to it here, it might help others.
If your issue is subsequently confirmed as a bug, and the report follows the issue template, it can be reopened.

@support support bot closed this as completed Jan 17, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 17, 2020

Thanks for the details. You can implement such logic with the exposed API.

@castroCrea
Copy link
Author

Do you means by using Ref ?
I tried many thing until now, but can't make it work as autoComplete use the ref on the input, with out messing up with the rest of the component.

The simpler solution will be to had a keyDown event on Tab like Enter, no ?

So i think it's a feature and note really a support ?

https://github.com/mui-org/material-ui/blob/020b7469d9ef1b9b545a23a40565a2a9c2efc1fc/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js#L582

@support support bot removed the support: Stack Overflow Please ask the community on Stack Overflow label Jan 17, 2020
@oliviertassinari oliviertassinari changed the title Autocomplete onBlur [Autocomplete] Improve autoSelect logic Jan 17, 2020
@oliviertassinari oliviertassinari added the new feature New feature or request label Jan 17, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 17, 2020

@castroCrea Looking closer, it seems that the auto-select logic is missing a few cases. Thanks for opening this issue.

  1. The autoSelect + multiple props combination is broken, it throws
  2. The autoSelect + freeSolo props combination is broken, it should auto-select, as it does on Gmail

What do you think of this patch (it would also fix autoSelect + multiple + freeSolo)?

diff --git a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
index a0dcae57a..d0b0fc444 100644
--- a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
@@ -649,7 +649,9 @@ export default function useAutocomplete(props) {
     }

     if (autoSelect && selectedIndexRef.current !== -1) {
-      handleValue(event, filteredOptions[selectedIndexRef.current]);
+      selectNewValue(event, filteredOptions[selectedIndexRef.current]);
+    } else if (autoSelect && freeSolo && inputValue !== '') {
+      selectNewValue(event, inputValue, 'freeSolo');
     } else if (!freeSolo) {
       resetInputValue(event, value);
     }

Do you want to work on a pull request? (as well as the corresponding tests) :)

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Jan 17, 2020
@castroCrea
Copy link
Author

No Problem,
I don't dig into the useAutocomplete as far as that, but it seems good.

I don't have a lot of time now but I can work on the PR later.

I couldn't find any test on autoComplete in the Test Folder.

@oliviertassinari
Copy link
Member

@castroCrea Great. If you don't have a lot of time, the most important part would be to confirm that this patch solves your problem, then, maybe a future developer facing the same problem, we take on the task carry it to a stable release. For the test, you can find them in Autocomplete/Autocomplete.test.js.

@captain-yossarian
Copy link
Contributor

@castroCrea if you don't mind I can work on it.

@castroCrea
Copy link
Author

no I don't mind, actually it will be nice. I don't have time yet.

@captain-yossarian
Copy link
Contributor

@castroCrea could you please tell me which versions of @material-ui/core and @material-ui/lab did you use?

@castroCrea
Copy link
Author

I use 4.8.3, the last one

@captain-yossarian
Copy link
Contributor

@castroCrea
"@material-ui/core": "4.8.3", "@material-ui/lab": "^4.0.0-alpha.39",
?

@castroCrea
Copy link
Author

yes exactly

@castroCrea
Copy link
Author

@captain-yossarian
Copy link
Contributor

@castroCrea I just want to make sure I've understand the issue.
New value should be saved, if it is not empty, on blur event?

@castroCrea
Copy link
Author

Yes for free solo and multiple

@captain-yossarian
Copy link
Contributor

@oliviertassinari your solution works if autoSelect is true (it is false by default) . I think adding new flag is a bad idea since there already a lot of props. Can I just remove this flag from condition? I'm not aware of the project, so it is hard to make decision

@oliviertassinari
Copy link
Member

@SerhiiBilyk We could change the default value of autoSelect, but it's a discussion for another issue. Here, I think that we should focus on making it work based on what we could expect from the existing API.

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work and removed new feature New feature or request labels Jan 25, 2020
@Cristy94
Copy link

Cristy94 commented Mar 7, 2020

Is the fix present in the latest version? The onChange still doesn't seem to trigger when bluring the input with freeSolo.

LE: Nevermind, autoSelect has to be set to true as mentioned above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: autocomplete This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants