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] Update autoSelect prop description #36280

Merged
merged 7 commits into from
Mar 28, 2023

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Feb 21, 2023

closes #30521

@sai6855 sai6855 marked this pull request as draft February 21, 2023 06:33
@mui-bot
Copy link

mui-bot commented Feb 21, 2023

Netlify deploy preview

https://deploy-preview-36280--material-ui.netlify.app/

Bundle size report

No bundle size changes

Generated by 🚫 dangerJS against 4e0edd0

@sai6855 sai6855 marked this pull request as ready for review February 21, 2023 07:23
*
* Set to `true` if you want to save the value when input looses focus.
* Set to `false` if you don't want to save the value when input looses focus.
* @default false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

kept default value as false to not break existing behaviour

@@ -935,7 +936,7 @@ export default function useAutocomplete(props) {

if (autoSelect && highlightedIndexRef.current !== -1 && popupOpen) {
selectNewValue(event, filteredOptions[highlightedIndexRef.current], 'blur');
} else if (autoSelect && freeSolo && inputValue !== '') {
} else if ((autoSelect || saveOnBlur) && freeSolo && inputValue !== '') {
Copy link
Member

Choose a reason for hiding this comment

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

@sai6855 This makes me think why not use autoSelect?

Copy link
Contributor Author

@sai6855 sai6855 Feb 21, 2023

Choose a reason for hiding this comment

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

@siriwatknp In reality, using autoSelect to save inputValue on blur works, but description of autoSelect in docs says, autoSelect adds highlighted option to value. docs nowhere indicates autoSelect adds inputValue to value. so maybe should we update docs instead of adding new prop?
Screenshot 2023-02-21 at 2 32 11 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we go in updation of autoSelect prop description direction, it would be great if you can provide appropriate description, just to avoid further confusion

Copy link
Member

Choose a reason for hiding this comment

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

From what I tested, adding a description sounds better than adding one more prop that likely does the same thing.

What do you think about this:

/**
* If `true`, the selected option becomes the value of the input
* when the Autocomplete loses focus unless the user chooses
* a different option or changes the character string in the input.
* 
* In addition of `freeSolo` mode, the selected option will the input value
* if the Autocomplete loses focus without highlighing an option.
* @default false
*/
autoSelect?: boolean;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • In addition of freeSolo mode, the selected option will the input value
  • if the Autocomplete loses focus without highlighing an option.

this part is bit unclear to me, if the Autocomplete loses focus **without** highlighing an option then which option will be the value of input as none of options are highlighted ?

should it be ?

* In addition of `freeSolo` mode, the typed value will be the input value
* if the Autocomplete loses focus without highlighing an option.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@siriwatknp updated description

@zannager zannager added the component: autocomplete This is the name of the generic UI component, not the React module! label Feb 21, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 21, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 4, 2023
@sai6855 sai6855 changed the title [Autocomplete] Adds saveOnBlur prop [Autocomplete] update autoSelect prop description Mar 4, 2023
@sai6855 sai6855 changed the title [Autocomplete] update autoSelect prop description [Autocomplete] Update autoSelect prop description Mar 4, 2023
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍

@@ -66,6 +66,9 @@ export interface UseAutocompleteProps<
* If `true`, the selected option becomes the value of the input
* when the Autocomplete loses focus unless the user chooses
* a different option or changes the character string in the input.
*
* In addition of `freeSolo` mode, the typed value will be the input value
Copy link
Member

Choose a reason for hiding this comment

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

I don't think I understand what this is saying. Is it meant to be "In addition to freeSolo mode", or "When using freeSolo mode"?

Copy link
Contributor Author

@sai6855 sai6855 Mar 27, 2023

Choose a reason for hiding this comment

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

i think when using freeSolo mode sounds much better, as autocomplete saves typed value only in freeSolo mode

Copy link
Member

Choose a reason for hiding this comment

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

ok that makes sense to me now, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samuelsycamore updated prop description in this commit 4e0edd0

@sai6855 sai6855 mentioned this pull request Mar 28, 2023
1 task
@mj12albert mj12albert merged commit a3d69a1 into mui:master Mar 28, 2023
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.

[Autocomplete] Save onBlur (free solo)
6 participants