Skip to content
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

Jump to bottom

CustomSelectControl: Add adapter for legacy to new version #57000

Closed
wants to merge 17 commits into from
Closed

CustomSelectControl: Add adapter for legacy to new version #57000

wants to merge 17 commits into from

Conversation

brookewp
Copy link
Contributor

@brookewp brookewp commented Dec 13, 2023 •
edited
Loading

What?

Part of #55023

Note

WIP: This is a WIP, and some pieces may be separated into other PRs if possible. For now, this is where all the experiments are happening.

Why?

To transition away from using CustomSelectControl to the new version for more flexibility.

How?

Inspired by ColorPicker, this looks at the props to determine if it's the new version or the legacy version of CustomSelectControl. If it's the legacy, then the props are translated into the new props, so the new ariakit version can be utilized instead.

No changes were made to CustomSelect or to CustomSelectItem, aside from moving them to their own files.

This also adds the same tests from CustomSelectControl. The only changes made were to the role which is now combobox instead of button and the tests for the custom event handlers have been removed.

Follow-up PR

  • Mark legacy props as to be deprecated
  • Rename component

Tests

  • Add assertions for value
  • Add additional tests for new component features
    • multi-select
    • rendering children

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

@brookewp brookewp added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Dec 13, 2023
@brookewp brookewp self-assigned this Dec 13, 2023
brookewp
brookewp commented Dec 13, 2023
View reviewed changes
packages/components/src/custom-select-control-v2/test/index.tsx
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially added the tests as is with a ts-nocheck in this commit: 1c39343

So it would be easier to see the changes required for the tests here: c011448

brookewp
brookewp commented Dec 13, 2023
View reviewed changes
packages/components/src/custom-select-control-v2/test/index.tsx Outdated
expect( currentSelectedItem ).toHaveTextContent( 'amber' );
} );

it( 'Can change selection with a focused input and closed dropdown if typed characters match an option', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There has been some flakiness with this test and I'm wondering if there is some leaky tests. Will be checking this as this moves along

brookewp
brookewp commented Dec 13, 2023
View reviewed changes
packages/components/src/custom-select-control-v2/use-deprecated-props.tsx
Copy link
Contributor Author

@brookewp brookewp Dec 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Still need to mark the props as to be deprecated, if we decide we want to go with this option

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 13, 2023 •
edited
Loading

Flaky tests detected in ee89e01.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7494430548
📝 Reported issues:

@brookewp brookewp force-pushed the try/adapt-legacy-customselectcontrol branch 2 times, most recently from 9a6874c to 79116aa Compare December 15, 2023 23:51
@brookewp brookewp force-pushed the try/adapt-legacy-customselectcontrol branch from 93f8d99 to d9c13ab Compare January 5, 2024 04:41
brookewp
brookewp commented Jan 5, 2024
View reviewed changes
packages/components/src/custom-select-control-v2/use-deprecated-props.tsx Outdated
Comment on lines 93 to 95
// temp ignore
//@ts-ignore
defaultValue: props?.value?.name ?? undefined,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have a type error here related to the Option type — reminder to look into this so it can be removed

@ciampo ciampo requested review from ciampo and removed request for ciampo January 8, 2024 17:42
@brookewp brookewp requested a review from ciampo January 8, 2024 17:43
This was referenced Jan 10, 2024
Closed
Closed
@brookewp brookewp force-pushed the try/adapt-legacy-customselectcontrol branch from 942f021 to 48333c9 Compare January 11, 2024 20:56
@brookewp brookewp force-pushed the try/adapt-legacy-customselectcontrol branch from 48333c9 to ee89e01 Compare January 11, 2024 20:56
This was referenced Jan 17, 2024
Merged
Closed
@brookewp
Copy link
Contributor Author

brookewp commented Feb 5, 2024

Closing in favour of #57902

@brookewp brookewp closed this Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ciampo ciampo Awaiting requested review from ciampo

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra will be requested when the pull request is marked ready for review ajitbohra is a code owner

Assignees

@brookewp brookewp

Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@brookewp