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] Add forcePopupIcon prop #18486

Closed
chrismcv opened this issue Nov 21, 2019 · 9 comments · Fixed by #18886
Closed

[Autocomplete] Add forcePopupIcon prop #18486

chrismcv opened this issue Nov 21, 2019 · 9 comments · Fixed by #18886
Labels
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. new feature New feature or request

Comments

@chrismcv
Copy link
Contributor

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

Summary 💡

The freeSolo property affects whether change events are fired when the input text does not match an item in the list. It also controls whether the AutoComplete renders as a Select or TextField. I think these should be controlled independently. freeSolo is also a non-intuitive name for the behaviour.

Examples 🌈

Textbox Autocomplete
image

Combobox selection
image

Motivation 🔦

Coming from a windows background, a ComboBox will still have the caret to pull the menu and edit an existing value. A Textfield can also have an auto complete list which will only appear when the user type, and the control would render as a TextField.

I suggest alternative property names
allowCustomUserInput and allowUserMenuSelection, or alternatively a means of the user toggling the menu visibility from a custom component supplied via renderInput

@oliviertassinari
Copy link
Member

@chrismcv Do you have a reproduction? I don't understand what behavior you are referring to.

@chrismcv
Copy link
Contributor Author

                {freeSolo ? null : (
                  <IconButton
                    {...getPopupIndicatorProps()}
                    disabled={disabled}
                    aria-label={popupOpen ? closeText : openText}
                    title={popupOpen ? closeText : openText}
                    className={clsx(classes.popupIndicator, {
                      [classes.popupIndicatorOpen]: popupOpen,
                    })}
                  >
                    {popupIcon}
                  </IconButton>
                )}

This code hides the down arrow (caret) in freeSolo mode.

And this code uses the same prop to prevent user input that isn't from the list.

I think each should be controlled separately. As I want freeSolo input with a down arrow visual.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 21, 2019

Thank you for the extra detail!

So caret is used as a synonym of the arrow indicator in the context of this issue. Regarding your two examples. They don't seem to support your use case. What's the motivation for using a free solo input and the arrow icon at the same time? It seems to go against the description in the motivation section.

@chrismcv
Copy link
Contributor Author

In our application we have a "Title" Combobox, which has values like Mr, Mrs, Miss, Dr, Rev. It is quickest for users to be able to pick these from a dropdown, but occassionally they need to add a new value that isn't in the system lists, and we want them to be able to. If we use freeSolo mode, the arrow will dissappear which will confuse our users.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 23, 2019

@chrismcv What do you think of a forcePopupIcon prop, it would be null by default, accept a boolean value. If true, the arrow is visible, if false, hidden?

With this prop, you can support your Combobox + custom value use-case with forcePopupIcon={true}.

@oliviertassinari oliviertassinari added new feature New feature or request good first issue Great for first contributions. Enable to learn the contribution process. component: autocomplete This is the name of the generic UI component, not the React module! labels Nov 23, 2019
@chrismcv
Copy link
Contributor Author

Yes, that'd be ideal.

@oliviertassinari oliviertassinari changed the title [AutoComplete] freeSolo mode shouldn't affect caret behaviour [Autocomplete] Add forcePopupIcon prop Nov 27, 2019
@oliviertassinari
Copy link
Member

Ok, so we can think of a diff close to the following:

diff --git a/packages/material-ui-lab/src/Autocomplete/Autocomplete.d.ts b/packages/material-ui-lab/src/Autocomplete/Autocomplete.d.ts
index 767a3862d..f6ed2909b 100644
--- a/packages/material-ui-lab/src/Autocomplete/Autocomplete.d.ts
+++ b/packages/material-ui-lab/src/Autocomplete/Autocomplete.d.ts
@@ -67,6 +67,10 @@ export interface AutocompleteProps
    * The children stay within it's parent DOM hierarchy.
    */
   disablePortal?: boolean;
+  /**
+   * Force the visibility display of the popup icon.
+   */
+  forcePopupIcon?: true | false | 'auto';
   /**
    * The component used to render the listbox.
    */
diff --git a/packages/material-ui-lab/src/Autocomplete/Autocomplete.js b/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
index b3f87c525..8febe1025 100644
--- a/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
+++ b/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
@@ -201,6 +201,7 @@ const Autocomplete = React.forwardRef(function Autocomplete(props, ref) {
     disablePortal = false,
     filterOptions,
     filterSelectedOptions = false,
+    forcePopupIcon = 'auto',
     freeSolo = false,
     getOptionDisabled,
     getOptionLabel = x => x,
@@ -340,7 +341,7 @@ const Autocomplete = React.forwardRef(function Autocomplete(props, ref) {
                   </IconButton>
                 )}

-                {freeSolo ? null : (
+                {(!freeSolo || forcePopupIcon === true) && forcePopupIcon !== false ? (
                   <IconButton
                     {...getPopupIndicatorProps()}
                     disabled={disabled}
@@ -352,7 +353,7 @@ const Autocomplete = React.forwardRef(function Autocomplete(props, ref) {
                   >
                     {popupIcon}
                   </IconButton>
-                )}
+                ) : null}
               </div>
             ),
           },

@SandraMarcelaHerreraArriaga
Copy link
Contributor

@oliviertassinari can I start to work with this issue?

@oliviertassinari
Copy link
Member

@SandraMarcelaHerreraArriaga Sure, the feature looks good.

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! good first issue Great for first contributions. Enable to learn the contribution process. new feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants