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

feat(material/autocomplete): add input to require selection from the panel #27423

Merged
merged 1 commit into from
Jul 11, 2023

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Jul 9, 2023

Adds the requireSelection input to the autocomplete, which when enabled will clear the input value if the user doesn't select an option from the list.

Fixes #3334.

@crisbeto crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: minor This PR is targeted for the next minor release labels Jul 9, 2023
@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Jul 9, 2023
@crisbeto crisbeto force-pushed the 3334/autocomplete-require-selection branch from a0bfd48 to deb61a5 Compare July 9, 2023 12:30
@crisbeto crisbeto self-assigned this Jul 11, 2023
…panel

Adds the `requireSelection` input to the autocomplete, which when enabled will clear the input value if the user doesn't select an option from the list.

Fixes angular#3334.
@crisbeto crisbeto force-pushed the 3334/autocomplete-require-selection branch from deb61a5 to a4403f7 Compare July 11, 2023 12:01
@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Jul 11, 2023
@crisbeto crisbeto merged commit af1a041 into angular:main Jul 11, 2023
9 checks passed
@alixroyere
Copy link

Thanks for this desired feature 😸

I've got some interrogations.
With this parameter set, does formcontrol valuechanges only emit valid (available in the selection, like a selector) values or every (invalid/partial string) value typed by the user?
If not, is the formControl validity set to false for these values?
It seems to only clean the value at the end of the panel usage. That may create some issues when using the parameter, because some not-selected values would be pushed in the control, which may be unexpected.

@crisbeto
Copy link
Member Author

It'll still receive the values typed by the user, but the value will be cleared if the panel is closed without selecting an option. I was considering not pushing values to the model at all, but that would introduce issues in the case where the input value is also used for filtering the options.

@alixroyere
Copy link

I understand but aren't these values available on the html input anyway, as with the (input) event?

@crisbeto
Copy link
Member Author

crisbeto commented Jul 11, 2023

They are, but usually the expectation when using reactive forms is that you wouldn't have to interact with the DOM directly. In retrospect, we should've had two form controls: one for the input and one for the autocomplete panel, instead of combining them into one, but making the change at this point would be a massive breaking change.

@alixroyere
Copy link

You're right 😿 That would have been easier to use as a consumer.
Then I don't know how to use this new feature as is, because form with required value would be valid when user is typing and panel is open. That would lead to weird behaviors. The only option I see, is to set a custom validator (checking if current value is inside the possible values), which is heavier than some propositions available in the initial issue here #3334 even if these propositions have their own issues too.
I wish something easy to use could be found

@crisbeto
Copy link
Member Author

There's still some time until the feature makes it into a release so we have time to iterate on it, but it's unclear to me what the alternative would be. We could add a custom validator in the autocomplete that only becomes invalid once the panel is closed?

@alixroyere
Copy link

You mean that becomes invalid once the panel is open, and valid once closed? That could help.

@crisbeto
Copy link
Member Author

Yeah basically it would be invalid only if the panel is closed and none of the option values is selected.

@alixroyere
Copy link

Or add a new input that would set what to put in the control (the user typing + the selected/empty values = default) or (only the selected/empty values). Is it still a massive breaking change?

@alixroyere
Copy link

alixroyere commented Jul 11, 2023

Or add a new input that would set what to put in the control (the user typing + the selected/empty values = default) or (only the selected/empty values). Is it still a massive breaking change?

Well in fact this is more an "AND" than an "OR". Because the validity issue would still be here for the combination of "requiredSelection=true" + default value for the other new parameter

@alixroyere
Copy link

Yeah basically it would be invalid only if the panel is closed and none of the option values is selected.

Are you're talking about the validity calculated by the validator (if control is valid for this validator), or about the validity of the validator (if validator is removed or something)?

@crisbeto
Copy link
Member Author

I'm not sure I follow the last comment. The autocomplete trigger would provide itself as a validator that sets an error like matAutocompleteRequireSelection if the user didn't make a selection.

@alixroyere
Copy link

Then, would this error be there only when selection panel is open or after panel close too?
In fact the current requireSelection behavior is cleaning the input after panel close, and the invalid control issue would be there only when panel is open.

If this is also applied when panel is closed:

  • Would the error be there for null/no selected value? We could need to require the setted value to be among options, but not require a value to be set.
  • What would happen to this error when value is set programmatically formControl.patchValue(value)?

@crisbeto
Copy link
Member Author

The error would be there if the panel is closed and the value is null. Since it's cleared when the user closes without selecting anything, that should guarantee that it's among the options. That won't catch invalid values set through patchValue, but at that point the application author has control over what value is being set so they can ensure that invalid values aren't being set.

@alixroyere
Copy link

alixroyere commented Jul 12, 2023

I was more thinking about setting a static invalid state as long as panel is open, and remove it once panel is closed. That would lead to more consistent behaviours, with requireSelection auto cleanings

@alixroyere
Copy link

alixroyere commented Jul 12, 2023

Well in fact what you propose may be better to match the input name "requireSelection". But some real life usages need to only "forceSelection" on existing values, not to require one to be set. Could there be something to cover this case?
If the new parameter was more as a forceSelection, it could still be used as a requireSelection when adding the classic required control parameter

@crisbeto
Copy link
Member Author

Capturing what we talked about in our team meeting: we should make it so the model value is null while typing and requireSelection is enabled.

@alixroyere
Copy link

nice !

stephenrca pushed a commit to stephenrca/components that referenced this pull request Aug 2, 2023
…panel (angular#27423)

Adds the `requireSelection` input to the autocomplete, which when enabled will clear the input value if the user doesn't select an option from the list.

Fixes angular#3334.
crisbeto added a commit to crisbeto/material2 that referenced this pull request Aug 3, 2023
…when requireSelection is enabled

Follow-up to angular#27423 based on the feedback. Usually `mat-autocomplete` assigns to the model as the user is typing which may not be desired when `requireSelection` is enabled, because at the end of the selection either an option value will set or it'll be reset.

These changes add a condition so that the value isn't assigned while typing and `requireSelection` is enabled.
@crisbeto
Copy link
Member Author

crisbeto commented Aug 3, 2023

I've sent out #27572 based on the discussion here. It's worth noting that I decided to keep the model value while the user is typing, instead of resetting it to null, because resetting it would emit an additional change event.

crisbeto added a commit to crisbeto/material2 that referenced this pull request Aug 3, 2023
…when requireSelection is enabled

Follow-up to angular#27423 based on the feedback. Usually `mat-autocomplete` assigns to the model as the user is typing which may not be desired when `requireSelection` is enabled, because at the end of the selection either an option value will set or it'll be reset.

These changes add a condition so that the value isn't assigned while typing and `requireSelection` is enabled.
crisbeto added a commit that referenced this pull request Aug 3, 2023
…when requireSelection is enabled (#27572)

Follow-up to #27423 based on the feedback. Usually `mat-autocomplete` assigns to the model as the user is typing which may not be desired when `requireSelection` is enabled, because at the end of the selection either an option value will set or it'll be reset.

These changes add a condition so that the value isn't assigned while typing and `requireSelection` is enabled.

(cherry picked from commit 77ffdf9)
crisbeto added a commit that referenced this pull request Aug 3, 2023
…when requireSelection is enabled (#27572)

Follow-up to #27423 based on the feedback. Usually `mat-autocomplete` assigns to the model as the user is typing which may not be desired when `requireSelection` is enabled, because at the end of the selection either an option value will set or it'll be reset.

These changes add a condition so that the value isn't assigned while typing and `requireSelection` is enabled.
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker detected: feature PR contains a feature commit P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Autocomplete] Restrict selection to given set of items
3 participants