feat(SelectControl): add children prop

[frosso]

Description

It looks like the select-control component accepts an options prop, which is just an array of options that are mapped to <option /> elements.
<optgroup />s are a native part of JSX. However, they cannot be added through the current options prop.

I understand that options is provided as an abstraction, but I think that rather than changing the options prop, we can leverage the native children prop present on all the JSX elements and obtain the same result.
We could still leverage the options prop to add <optgroup /> elements, but where's the fun in that? It seems to me that by leveraging the children prop we can have a more semantical component. And with that we can just leverage <optgroup />s natively!

I kept the options prop, but maybe it can deprecated in favor of children?

How has this been tested?

I tested this in Storybook. I also added some unit tests for the component (there weren't any before).

Screenshots

https://d.pr/i/JvrB57

Types of changes

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• I've tested my changes with keyboard and screen readers.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR.

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @frosso! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[talldan]

Thanks for your contribution @frosso.

This sounds like a reasonable change, and probably how many would expect the component to work. From memory, I think there are other components that work like this too (maybe DropdownMenu?)

I think as part of this the README.md would need to be updated. Some of the docs may (or may not) need to be generated after modifying that file (npm run docs:build and then also commit and push any generated changes).

[talldan]

A convention in the codebase is that test file name should try to map to the filename that's being tested in the parent folder, so here it should be called index.js (and it doesn't need the test.js postfix).

[frosso]

Thanks - done in 6c4e3a2

[frosso]

Thanks for taking the time to review @talldan !
I made an update to the README.md in 18872a4. Running npm run docs:build didn't seem to create additional changes.
Let me know if there's anything else!

[frosso]

Thanks - done in 6c4e3a2

[ajlende]

I resolved the conflicts with trunk and everything works as expected. Looks like @talldan's comments have all been resolved too, so I think this looks good to go 👍

[ciampo]

Hey @frosso , it looks like this change introduced some typescript errors in the package, since the compiler now thinks that the children prop is mandatory on SelectControl (for example, here) — would you be able to work on a fix to mark children as optional?

Something like this should be enough

diff --git a/packages/components/src/select-control/index.tsx b/packages/components/src/select-control/index.tsx
index 7f68f56721..ec2afed60d 100644
--- a/packages/components/src/select-control/index.tsx
+++ b/packages/components/src/select-control/index.tsx
@@ -4,7 +4,7 @@
 import { isEmpty, noop } from 'lodash';
 import classNames from 'classnames';
 // eslint-disable-next-line no-restricted-imports
-import type { ChangeEvent, FocusEvent, Ref } from 'react';
+import type { ChangeEvent, FocusEvent, ReactNode, Ref } from 'react';
 
 /**
  * WordPress dependencies
@@ -31,7 +31,7 @@ function useUniqueId( idProp?: string ) {
 }
 
 export interface SelectControlProps
-	extends Omit< InputBaseProps, 'isFocused' > {
+	extends Omit< InputBaseProps, 'children' | 'isFocused' > {
 	help?: string;
 	hideLabelFromVision?: boolean;
 	multiple?: boolean;
@@ -50,6 +50,7 @@ export interface SelectControlProps
 	size?: Size;
 	value?: string | string[];
 	labelPosition?: LabelPosition;
+	children?: ReactNode;
 }
 
 function SelectControl(

Also, when making changes on the @wordpress/components package, we usually add an entry to the CHANGELOG. You can find more information regarding best practices in the contributing guidelines.

Finally, feel free to ping myself and/or @mirka in any future PR concerting @wordpress/components. Thank you!

[ciampo]

ReactNode would be a better choice for describing children 's type

[ciampo]

Opened #37872 with the fix suggested in the previous comment