material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

Typescript: Autocomplete value validation when using controlled value (empty first) but with disabledClearable on

Open Gr3yShad0w opened this issue 4 years ago • 3 comments

  • [ X] The issue is present in the latest release.
  • [ X] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

Im implementing the Autocompleted with controlled state (from react-hook-form). The Value in the beginning should be emtpy => null When the user selects an Item the ID of the item is the controlled value.

When providing value || null the typescript compiler throws an error because nullable values are not allowed when disableClearable is set. It makes sence that an updated value cannot be null but initialy it makes sence.

Expected Behavior 🤔

Allow null as value even when disableClearable is set.

Steps to Reproduce 🕹

@mui/core/AutocompleteUnstyled/useAutocomplete.d.ts

export type AutocompleteValue<T, Multiple, DisableClearable, FreeSolo> = Multiple extends
  | undefined
  | false
  ? DisableClearable extends true
    ? NonNullable<T | AutocompleteFreeSoloValueMapping<FreeSolo>>
    : T | null | AutocompleteFreeSoloValueMapping<FreeSolo>
  : Array<T | AutocompleteFreeSoloValueMapping<FreeSolo>>;

https://codesandbox.io/s/combobox-material-demo-forked-dd8de?file=/demo.tsx

Context 🔦

I have a form (react-hook-form) one field is userId which can be selected from a list of users The value is controlled and should contain the userId directly which is why I dont just pass the whole object (label + id) as value but rather select the option by value (id)

Gr3yShad0w avatar Oct 14 '21 10:10 Gr3yShad0w

Why don't you add the disableClearable once you have an option selected? Or add a default value for the autocomplete? I believe the current typings make sense.

mnajdova avatar Oct 26 '21 12:10 mnajdova

Because a behaviour of the component (remove a button that clears the input) should not make any impact on the value a componet can receive. The only thing it should change is that the update value now no longer can be null, but the value null should still be accepted

Gr3yShad0w avatar Nov 02 '21 13:11 Gr3yShad0w

I think there are a couple of problems with disableClearable and freeSolo if both are true you can have an empty value and it allows it. So I guess disabledClearable should be pointing to the clearing of input by the user only but the value still can be empty because it is not selected yet. If the value exists then it should be restored but it should only when freesolo false when you got freeSolo it can not be restored. What I can see are people mixing disabledClearable with required and its validation. So in the end disabledClearable only needs for displaying a clear button and restoring the value if it is not freeSolo but the value can be empty. Or need to separate disabledClearable with restoreValue(only if it been selected before) combination if field actually required so validation should handle that user must select something from the dropdown.

https://codesandbox.io/s/combobox-material-demo-forked-dcrlow?file=/demo.tsx

povilass avatar Aug 05 '22 14:08 povilass

Why don't you add the disableClearable once you have an option selected?

Is this:

<Autocomplete
  value={value || null}
  disableClearable={!!value}

the suggestion, essentially?

rmacklin avatar Jan 12 '23 03:01 rmacklin

What I can see are people mixing disabledClearable with required and its validation.

@povilass There can be some confusion, true. But in terms of UX it makes perfect sense: do not de-select the value once you've chosen one, because it's required anyway. And disabling only when a value is there sound like a very dirty hack. Although I don't know if it will be cleaner inside if one manages to change the current behaviour

Jazzmanpw avatar Mar 03 '23 18:03 Jazzmanpw