SelectControl: Pass through options props
#64211
Conversation
mirka
added
[Type] Enhancement
| @@ -26,6 +26,22 @@ function useUniqueId( idProp?: string ) { | |||
| return idProp || id; | |||
| } | |||
|
|
|||
| function SelectOptions( { | |||
There was a problem hiding this comment.
Extracted this out from the main component function because we need to pick props from the options.map and the names are going to conflict with the main component props.
| options: NonNullable< SelectControlProps[ 'options' ] >; | ||
| } ) { | ||
| return options.map( ( { id, label, value, ...optionProps }, index ) => { | ||
| const key = id || `${ label }-${ value }-${ index }`; |
There was a problem hiding this comment.
As a key, ${ label }-${ value }-${ index } seems functionally equivalent to index 🤔 Doesn't seem particularly intentional, either (#6122). I'm not touching this in this PR though.
| it( 'should render its children', async () => { | ||
| const user = userEvent.setup(); |
| id?: string; | ||
| /** | ||
| * Whether or not the option should have the disabled attribute. | ||
| * | ||
| * @default false | ||
| */ | ||
| disabled?: boolean; | ||
| /** | ||
| * Whether or not the option should be hidden. | ||
| * | ||
| * @default false | ||
| */ | ||
| hidden?: boolean; |
There was a problem hiding this comment.
Removing these explicit types since they are included in React.OptionHTMLAttributes.
| onBlur?: ( event: FocusEvent< HTMLSelectElement > ) => void; | ||
| onFocus?: ( event: FocusEvent< HTMLSelectElement > ) => void; |
There was a problem hiding this comment.
Removing these explicit event listener props since they are mixed in here:
gutenberg/packages/components/src/select-control/index.tsx
Lines 45 to 47 in 2c01c7d
This will remove them from the props table and auto-generated code snippets in Storybook, which is good because we have no reason to explicitly show onBlur and onFocus.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| /** | ||
| * The value of the selected option. | ||
| * | ||
| * If `multiple` is true, the `value` should be an array with the values of the selected options. | ||
| */ |
There was a problem hiding this comment.
Does it make sense to make this more specific, so it's clear it's for a single select? TypeScript can often see if the component has multiple or not and can determine which description to display in the IDE properly:
There was a problem hiding this comment.
I'd love to, but I've been avoiding writing different descriptions for "conditional" props, because they don't work well with the Storybook props table. They simply get concatenated when they differ:
There was a problem hiding this comment.
That's not a great reason for the type description verbosity TBH 😞 I wish Storybook could handle that in a more clever way. Are we the first to have that problem? Seems like this is similar to what we need: storybookjs/storybook#21281
There was a problem hiding this comment.
I don't necessarily dislike this limitation though. I think it is useful to know that a prop is conditionally polymorphic.
| /** | ||
| * The value of the selected option. | ||
| * | ||
| * If `multiple` is true, the `value` should be an array with the values of the selected options. | ||
| */ |
There was a problem hiding this comment.
As above: Does it make sense to make this more multiple-specific, so it's clear that it's always an array of values and not a single value since it's only when the select field is a multiple select one?
Prerequisite for #63815
What?
In
SelectControl, pass through any extra props defined inoptionsto the underlying<option>element.Why?
To support any additional
optionattributes, for examplearia-label.Testing Instructions
In Storybook, pass some additional
optionattributes and see what gets rendered in HTML.✅ Unit test passes