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 1 commit
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
Prev Previous commit
Next Next commit
Merge branch 'trunk' into meteorlxy/custom-select-option-type
  • Loading branch information
@meteorlxy
meteorlxy authored Jul 27, 2024
commit 7c28ddfbcf7d7db240dd4e34cd042aa38cb29d65
4 changes: 4 additions & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Bug Fixes

- `CustomSelectControl`: Restore `describedBy` functionality ([#63957](https://github.com/WordPress/gutenberg/pull/63957)).

### Enhancements
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.

Since this is a type change, I believe it should be under Internal instead.

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.

Cuz the type will be used by user, and will improve the developer experience, I think it's more of an improvement / enhancement?

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.

IMHO, types and type changes are an internal kind of enhancement, and we have this "Internal" group to distinguish them. General Enhancements would usually be targeted towards end users.


- `CustomSelectControl`: Support generic props type ([#63985](https://github.com/WordPress/gutenberg/pull/63985)).
Expand Down
10 changes: 10 additions & 0 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 @@ -17,6 +17,7 @@ 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, CustomSelectOption } from './types';
import { VisuallyHidden } from '../visually-hidden';

function useDeprecatedProps< T extends CustomSelectOption >( {
__experimentalShowSelectedHint,
Expand All @@ -41,6 +42,15 @@ function applyOptionDeprecations( {
};
}

function getDescribedBy( currentValue: string, describedBy?: string ) {
if ( describedBy ) {
return describedBy;
}

// translators: %s: The selected option.
return sprintf( __( 'Currently selected: %s' ), currentValue );
}

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 >
) {
Expand Down
You are viewing a condensed version of this merge commit. You can view the full changes here.