[SelectUnstyled] Add ability to post the select's value when submitting a form #33697
Conversation
|
@material-ui/unstyled: parsed: +0.67% , gzip: +0.53% |
michaldudak
force-pushed
the
select-form-submit
branch
from
71b115d
to
57bda52
Compare
michaldudak
added
component: select
| * Used to set a value of a hidden input associated with the select, | ||
| * so that the selected value can be posted with a form. | ||
| */ | ||
| formValueProvider?: (option: SelectOption<TValue> | null) => string | number; |
There was a problem hiding this comment.
Why is this here string | number, but in the MultiSelect it is only string | string[]. Should this be string only?
There was a problem hiding this comment.
Not sure on the name. Should it be simply getValue?
There was a problem hiding this comment.
Why is this here string | number, but in the MultiSelect it is only string | string[]. Should this be string only?
The native select's (and hidden input's as well) value is typed as string | number | readonly string[] | undefined. We don't have to artificially restrict numbers.
Should it be simply getValue?
getValue doesn't indicate it's a value serialized for a form. Perhaps serializeValue (or getSerializedValue, or getFormValue)?
There was a problem hiding this comment.
getSerializedValue sounds great 👍
The native select's (and hidden input's as well) value is typed as string | number | readonly string[] | undefined. We don't have to artificially restrict numbers.
Can we just use this type then? We can omit some values in the single select
There was a problem hiding this comment.
Good point. I suppose we don't even need to omit values in a single select - if someone really wants to convert a single value to string[], there are no technical reasons why they shouldn't.
| /** | ||
| * Callback fired when an option is selected. | ||
| */ | ||
| onChange?: (value: TValue | null) => void; | ||
| /** | ||
| * A function used to convert the option value to a string. | ||
| * It is used to navigate through values with keyboard. |
There was a problem hiding this comment.
I don't really understand from the description what the prop is used for and how it is different than formValueProvider
| * | ||
| * @default defaultOptionStringifier | ||
| */ | ||
| optionStringifier: PropTypes.func, |
There was a problem hiding this comment.
| optionStringifier: PropTypes.func, | |
| getStringifiedOption: PropTypes.func, |
Maybe? I am not sure, but I would use getX as a prefix.
There was a problem hiding this comment.
I'm fine with both. It's an existing parameter of useSelect, however, so I wouldn't rename it in this PR.
| z-index: 1; | ||
| `; | ||
|
|
||
| const Header = styled('h1')( |
There was a problem hiding this comment.
|
|
||
| function CustomSelect(props) { | ||
| const components = { | ||
| Root: StyledButton, |
There was a problem hiding this comment.
Are we sure about the use of a <button>? In https://www.w3.org/WAI/ARIA/apg/example-index/combobox/combobox-select-only.html, it's a <div role="combobox">. Curiously, all the other libraries I can benchmark use a button like us.
There was a problem hiding this comment.
Unless it causes any issues, I'd like to keep it this way. The link you mentioned points to an example, not reference documentation, so I assume it's not an error to use a button here.
When a
nameprop is provided, the (Multi)SelectUnstyled component renders a hidden input holding the currently selected value(s).As the value of a hidden input must be a string, the new
formValueProviderprop was added to control the serialization of the (Multi)Select's value.Closes #30992