Skip to content

CustomSelectControl: support generic props type #63985

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jul 29, 2024
Merged

CustomSelectControl: support generic props type #63985

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4f5c724
CustomSelectControl: support generic props type
meteorlxy Jul 26, 2024
478581c
chore: update changelog
meteorlxy Jul 26, 2024
7c28ddf
Merge branch 'trunk' into meteorlxy/custom-select-option-type
meteorlxy Jul 27, 2024
162aa1d
chore: tweak changelog
meteorlxy Jul 29, 2024
1aaf455
Merge remote-tracking branch 'origin/trunk' into meteorlxy/custom-sel…
meteorlxy Jul 29, 2024
19c5f92
chore: tweak ordering
meteorlxy Jul 29, 2024
987a017
chore: remove export
meteorlxy Jul 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
### Internal

- `DropdownMenuV2`: break menu item help text on multiple lines for better truncation. ([#63916](https://github.com/WordPress/gutenberg/pull/63916)).
- `CustomSelectControl`: Support generic props type ([#63985](https://github.com/WordPress/gutenberg/pull/63985)).

## 28.4.0 (2024-07-24)

Expand Down
12 changes: 7 additions & 5 deletions packages/components/src/custom-select-control/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,13 @@ import { __, sprintf } from '@wordpress/i18n';
import _CustomSelect from '../custom-select-control-v2/custom-select';
import CustomSelectItem from '../custom-select-control-v2/item';
import * as Styled from '../custom-select-control-v2/styles';
import type { CustomSelectProps } from './types';
import type { CustomSelectProps, CustomSelectOption } from './types';
import { VisuallyHidden } from '../visually-hidden';

function useDeprecatedProps( {
function useDeprecatedProps< T extends CustomSelectOption >( {
__experimentalShowSelectedHint,
...otherProps
}: CustomSelectProps ) {
}: CustomSelectProps< T > ) {
return {
showSelectedHint: __experimentalShowSelectedHint,
...otherProps,
Expand All @@ -35,7 +35,7 @@ function useDeprecatedProps( {
function applyOptionDeprecations( {
__experimentalHint,
...rest
}: CustomSelectProps[ 'options' ][ number ] ) {
}: CustomSelectOption ) {
return {
hint: __experimentalHint,
...rest,
Expand All @@ -51,7 +51,9 @@ function getDescribedBy( currentValue: string, describedBy?: string ) {
return sprintf( __( 'Currently selected: %s' ), currentValue );
}

function CustomSelectControl( props: CustomSelectProps ) {
function CustomSelectControl< T extends CustomSelectOption >(
Copy link
Member

@tyxla tyxla Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a default to the generic so any existing consumers will continue working without changing anything?

The same question applies to all instances where we're adding a generic and there wasn't one before.

Copy link
Contributor Author

@meteorlxy meteorlxy Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generic param is not going to be set manually by user. It's used for type inferring. To be specific, it will use the item type of the options prop automatically. So consumers would not need to change anything.

You could checkout this branch and test with the snippets that pasted in the "Testing Instructions" section.

Copy link
Member

@jsnajdr jsnajdr Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeScript should always be able to infer the T type. CustomSelectProps have a mandatory options: Array<T> field, so T is always the type of whatever you pass as options, it's easy to infer. TypeScript will merely check if that type is compatible with CustomSelectOption.

props: CustomSelectProps< T >
) {
const {
__next40pxDefaultSize = false,
describedBy,
Expand Down
14 changes: 7 additions & 7 deletions packages/components/src/custom-select-control/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import type { FocusEventHandler, MouseEventHandler } from 'react';
/**
* The object structure for the options array.
*/
type Option = {
export type CustomSelectOption = {
key: string;
name: string;
style?: React.CSSProperties;
Expand All @@ -24,15 +24,15 @@ type Option = {
/**
* The object returned from the onChange event.
*/
type ChangeObject = {
export type CustomSelectChangeObject< T extends CustomSelectOption > = {
Copy link
Member

@tyxla tyxla Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to export this one? Seems like we don't.

Copy link
Contributor Author

@meteorlxy meteorlxy Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be useful if user want to get the ChangeObject type, otherwise we have to use the following snippet to get the type:

image

Copy link
Member

@tyxla tyxla Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right 👍

Exporting it here doesn't export it from the package, though. And if we're not importing it inside the package, then it's effectively unnecessary to export it, IMHO. We can always start exporting it if we need to in the future.

Copy link
Member

@mirka mirka Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, at the moment we are avoiding public type exports as much as possible, thanks 🙏

Copy link
Contributor Author

@meteorlxy meteorlxy Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, removed

highlightedIndex?: number;
inputValue?: string;
isOpen?: boolean;
type?: string;
selectedItem: Option;
selectedItem: T;
};

export type CustomSelectProps = {
export type CustomSelectProps< T extends CustomSelectOption > = {
/**
* Optional classname for the component.
*/
Expand All @@ -55,7 +55,7 @@ export type CustomSelectProps = {
* Function called with the control's internal state changes. The `selectedItem`
* property contains the next selected item.
*/
onChange?: ( newValue: ChangeObject ) => void;
onChange?: ( newValue: CustomSelectChangeObject< T > ) => void;
/**
* A handler for `blur` events on the trigger button.
*
Expand Down Expand Up @@ -83,7 +83,7 @@ export type CustomSelectProps = {
/**
* The list of options that can be chosen from.
*/
options: Array< Option >;
options: Array< T >;
/**
* The size of the control.
*
Expand All @@ -93,7 +93,7 @@ export type CustomSelectProps = {
/**
* Can be used to externally control the value of the control.
*/
value?: Option;
value?: T;
/**
* Use the `showSelectedHint` property instead.
* @deprecated
Expand Down