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

Value for Select which allows to not select any of the options #17568

Closed
ghost opened this issue Sep 25, 2019 · 14 comments · Fixed by #17691
Closed

Value for Select which allows to not select any of the options #17568

ghost opened this issue Sep 25, 2019 · 14 comments · Fixed by #17691
Labels
component: select This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process. hacktoberfest https://hacktoberfest.digitalocean.com/

Comments

@ghost
Copy link

ghost commented Sep 25, 2019

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

Summary 💡

In this example: https://material-ui.com/components/selects/, you see on the first select component:

  1. No option is selected
  2. Even though options are loaded in the select component.

This is good and desired behavior. But this is only possible if user had set

 const [values, setValues] = React.useState({
    age: '',
    name: 'hai',
  });

age to empty string above (even though from the options none of the menu items had '' as value).

So it seems in react material setting value of select to empty string means that none of the provided options are selected - this is good, but nowhere does the documentation mentions this.

So can you add to the documentation that if you provide empty string as value to the Select, then none of the provided options will be selected?

(Otherwise I had weird issues today when the InputLabel was showing incorrectly when I initially forgot to specify empty string as value)

@oliviertassinari oliviertassinari added component: select This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation labels Sep 25, 2019
@oliviertassinari
Copy link
Member

@Giorgi-M How would you envision this to be documented?

@ghost
Copy link
Author

ghost commented Sep 25, 2019

@oliviertassinari This is problematic only if I have InputLabel and set as value some random value, see: https://codesandbox.io/s/material-demo-w5r5f

the label is shown on top, whereas if you set value to '', then the label isn't set on top anymore.

If there is no InputLabel, then even if I set value to some random number, this visual behavior isn't observed anymore.

So, given now I am a bit also confused about the logic how all this works, now I am confused too how to document it, but given what I said above, you could put in the docs where value is defined that if you want to not show selection just pass '' to it (but as I said without InputLabel even passing any other value than '' also seems to work).


So it seems '' has some special behavior to it. If that's correct, then just mention that in the docs where value parameter is explained (in API docs)

Or maybe change component behavior such that if I provide a value which isn't in the options, then the InputLabel should always be shown as normal (not on top) - same as it happens when I provide ''.

@oliviertassinari oliviertassinari added low priority and removed docs Improvements or additions to the documentation labels Sep 25, 2019
@ghost
Copy link
Author

ghost commented Sep 26, 2019

@oliviertassinari does this issue make sense, that for '' and random values (one which isn't in options), the InputLabel is shown differently?

@oliviertassinari
Copy link
Member

@Giorgi-M This is an interesting concern. The very first demo we have is basically a non-clearable select. I'm not aware of any equivalent behavior with the native select. We first designed the non-native select to be as close as possible to the native select behavior. It's definitely a divergence from this objective. However, it seems that Vuetify, Angular material, downshift select support this case too.

We are navigating on the boundaries of what the component supports, the mismatch between the value and the options available (out-of-range) is an edge case we don't support well.

The introduction of this logic:

  // Check the input state on mount, in case it was filled by the user
  // or auto filled by the browser before the hydration (for SSR).
  React.useEffect(() => {
    checkDirty(inputRef.current);
  }, []); // eslint-disable-line react-hooks/exhaustive-deps

Has improved the support for the out-of-range for the native select, as the native select change the value to the first option, without triggering a change event. However, nothing was done for th non-native select.

I can think of the following options:

  1. We raise a warning when an out-of-range value is found.
  2. We trigger a change event when an out-of-range value is found (it used to be what we do with the table pagination, we later down the road move to approach 1.).

What do you think of 1?

diff --git a/packages/material-ui/src/Select/SelectInput.js b/packages/material-ui/src/Select/SelectInput.js
index e636d6510..cc848f333 100644
--- a/packages/material-ui/src/Select/SelectInput.js
+++ b/packages/material-ui/src/Select/SelectInput.js
@@ -193,6 +193,8 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
     }
   }

+  let foundMatch = false;
+
   const items = React.Children.map(children, child => {
     if (!React.isValidElement(child)) {
       return null;
@@ -230,6 +232,10 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
       }
     }

+    if (selected) {
+      foundMatch = true;
+    }
+
     return React.cloneElement(child, {
       'aria-selected': selected ? 'true' : undefined,
       onClick: handleItemClick(child),
@@ -240,6 +246,22 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
     });
   });

+  if (process.env.NODE_ENV !== 'production') {
+    // eslint-disable-next-line react-hooks/rules-of-hooks
+    React.useEffect(() => {
+      if (!foundMatch && !multiple && value !== '') {
+        const child = React.Children.toArray(children)[0];
+        console.warn(
+          [
+            `Material-UI: you have provided an out-of-range value for the select (name="${name}") component.`,
+            'Consider providing a value that match one of the available options.',
+            `The first option value available is ${child.props.value}.`,
+          ].join('\n'),
+        );
+      }
+    }, [foundMatch, children, multiple, name, value]);
+  }
+
   if (computeDisplay) {
     display = multiple ? displayMultiple.join(', ') : displaySingle;
   }

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation and removed low priority labels Sep 26, 2019
@ghost
Copy link
Author

ghost commented Sep 26, 2019

@oliviertassinari To make sure we are on same page, let's conclude (from demo 1):

  1. The problem occurs when we use also InputLabel
  2. The problem is the label is shown on top of empty input, if an out of range value is provided (except when '' is given as value - even if it is out of range - then in that case, the label is shown within input)

So maybe if user gives out of range value, yeah give warning that "You used out of range value. Either give a correct value, or provide the component with empty string." because in that case the label is correctly positioned imho.

Actually, now I saw the component does have a similar warning if you give it null: "Consider using an empty string to clear the component or undefined for uncontrolled components."

So it seems currently '' is indication if you want to have Select with no options selected (If that's the case I'd also mention it in API docs, next to value).

I am not sure what the change handler solves as you suggested, does it select one of the options? That might not be too good, as IMHO it is nice if Select allows to be in a state where none of the values are chosen.

@oliviertassinari
Copy link
Member

Thanks for the feedback. I think that we are on the same page. For consistency, I plan to have the same behavior for the upcoming autocomplete component.

Do you want to work on a pull request? :)

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Sep 26, 2019
@ghost
Copy link
Author

ghost commented Sep 26, 2019

Thanks for the feedback. I think that we are on the same page. For consistency, I plan to have the same behavior for the upcoming autocomplete component.

you mean to let component select valid option in case of out of range value?

For pull request, sorry at the moment, I am having some busy times. Maybe next time.

In the mean time I would like to direct your attention to this issue:facebook/react#16901, which I opened elsewhere but seems to be problem with material's withWidth.

@oliviertassinari
Copy link
Member

I mean that we can add a warning about out of range values, similar to what we do with the Tabs component. It's also something we could consider doing with the RadioGroup (at the difference that we can't introspect the children prop, it would require to look at the DOM).

I also think that we can add a specific mention about value="" in the prop description. We could say that an empty string can be provided with non-option associated to support not clearable select.

@joshwooding joshwooding added the hacktoberfest https://hacktoberfest.digitalocean.com/ label Sep 30, 2019
@asownder95
Copy link
Contributor

I can submit a PR for this issue, if it's still needed!

@joshwooding
Copy link
Member

@asownder95 Yes please!

@stud-mai
Copy link

stud-mai commented Nov 7, 2019

I use FixedSizeList from react-window package inside of Select component and I receive such kind of warning:

Material-UI: you have provided an out-of-range value `UTC` for the select component.
Consider providing a value that matches one of the available options or ''.
The available values are "". 

which seems to me quite useless. Although everything works as expected it become annoying to get this warning on each item change in Select component. Is there any tweak or workaround not to bump into that warning?

@oliviertassinari
Copy link
Member

@stud-mai I think that we can move the concern to #14943.

@stud-mai
Copy link

stud-mai commented Nov 7, 2019

@oliviertassinari yeah, seems it's right place to move my concern

@skavan
Copy link

skavan commented Jul 18, 2020

Hi,
Forgive me if I'm wading in, in the wrong place.
I too am getting the "out-of-range" warning -- and while not a breaking issue - it is annoying.
There are cases where an out of range value is a perfectly acceptable outcome.

Example: I have a server monitoring application that persists a users preferred server (via a cookie). That preference is pushed to the redux store on application initialization.

In my settings form, the user picks (Formik--> field --> select) their preferred server from those that are dynamically provided by a network scan (and pushed into the redux store and thereafter presented as the list of menuitems in a field>select). It is entirely possible that the stored preferred server (cookies-->store--->Formik Initial Values) is no longer valid.

When one goes to the settings form, all is good. Validation (validateOnMount) does it's job and the field is flagged into error state. Except for the pesky littering of "out of range" warnings - that are generated because the field name in a Formik field is delivering a value that isn't in the Select MenuItems.

So - what's the solution? In my mind, it's having an OutOfRange handler property. If implemented, by the developer, the out-of-range warning is suppressed, because by definition, the user is aware of it. Under the hood, Formik can pass a non-valid value as '' if it needs to - to kill the warning.

So (and I'm horrible at explaining this stuff) -

  1. normally, Field, takes the name property, uses it against the values object and delivers the output to the field (which in turn is connection to the select options). It seems, currently, not to care if the value is invalid.
  2. If the dev implements and OutOfRange property/function...that function can deliver a display only value of '' - to clear the warning. The "bad" underlying value remains in the Formik values obj. The field is flagged as error.

s.

Note - in the above use case, the correct action is not to auto rewrite the value to ''...because the server may come on line...so having the conflict and being able to handle it without correcting it, is important.

Hope that makes vague sense.

s.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: select This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process. hacktoberfest https://hacktoberfest.digitalocean.com/
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants