CustomSelectControlV2: fix handling of extra attributes passed to options in the legacy adapter
#62255
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fullofcaffeine
changed the title
CustomSelectControlV2: Fix options handling in the legacy adapter
|
Size Change: +94 B (+0.01%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
fullofcaffeine
changed the title
CustomSelectControlV2: Fix selectedItem type in onChange callback for the legacy adapter
fullofcaffeine
changed the title
CustomSelectControlV2: Fixn selectedItem type in onChange callback for the legacy adapter
fullofcaffeine
force-pushed
the
fix/custom-select-control-v2-legacy-adapter-option-handling
branch
from
e5fdebb
to
c3e3c71
Compare
|
It looks like we'll need to support arbitrary properties ( I'm not sure how this would work for V2 - e.g., if V2 actually supports custom properties in an option like this. However, that's not the main focus of this PR, and we can discuss this in parallel. From what I see, for V2, consumers would use this API from Ariakit -> https://ariakit.org/reference/select-item#getitem? |
fullofcaffeine
force-pushed
the
fix/custom-select-control-v2-legacy-adapter-option-handling
branch
from
c3e3c71
to
6daa4b1
Compare
|
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. |
fullofcaffeine
added
[Package] Components
|
I will take over the work on this PR. I will first take a look at #61272 to get more context, then leave a review of the current status of this PR, and then start work on it |
ciampo
reviewed
Jun 20, 2024
packages/components/src/custom-select-control-v2/legacy-component/index.tsx
Outdated
Show resolved
Hide resolved
ciampo
reviewed
Jun 20, 2024
packages/components/src/custom-select-control-v2/legacy-component/index.tsx
Show resolved
Hide resolved
ciampo
reviewed
Jun 20, 2024
packages/components/src/custom-select-control-v2/legacy-component/index.tsx
Outdated
Show resolved
Hide resolved
ciampo
reviewed
Jun 20, 2024
packages/components/src/custom-select-control-v2/legacy-component/test/index.tsx
Outdated
Show resolved
Hide resolved
ciampo
reviewed
Jun 20, 2024
packages/components/src/custom-select-control-v2/legacy-component/test/index.tsx
Outdated
Show resolved
Hide resolved
ciampo
reviewed
Jun 20, 2024
ciampo
reviewed
Jun 20, 2024
ciampo
reviewed
Jun 20, 2024
ciampo
reviewed
Jun 20, 2024
| @@ -286,7 +297,7 @@ describe.each( [ | |||
| expect.objectContaining( { | |||
| inputValue: '', | |||
| isOpen: false, | |||
| selectedItem: { key: 'violets', name: 'violets' }, | |||
| selectedItem: { key: 'flower1', name: 'violets' }, | |||
There was a problem hiding this comment.
Need to double-check if this behaviour matches v1
There was a problem hiding this comment.
v1 doesn't behave this way, because it doesn't fire onChange on first render. This is something that we should investigate separately from this PR, though
ciampo
force-pushed
the
fix/custom-select-control-v2-legacy-adapter-option-handling
branch
from
6daa4b1
to
5882f00
Compare
|
Flaky tests detected in 594981f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9594432994
|
…tom attributes as part of option and that they are not added to the DOM as attributes (same behavior as V1)
ciampo
force-pushed
the
fix/custom-select-control-v2-legacy-adapter-option-handling
branch
from
c7530a3
to
192ac71
Compare
|
Most unit test changes were moved to #62706 and merged. I rebased, added a few more tweaks, and updated the tracking issue with the additional follow-up tasks that emerged while working on this PR. This PR is ready for a new round of review — hopefully we're able to merge it soon enough. I also updated the PR description and added some extra testing instructions for testing in the editor. |
ciampo
reviewed
Jun 20, 2024
There was a problem hiding this comment.
Changes in this file are simply to allow the v1 Storybook to log onChange arguments in the "Actions" tab, thus making it easier to debug the changes in Storybook
|
Follow-up PR fixing value setting is up and will be ready for final review as soon as this PR is merged |
tyxla
approved these changes
Jun 21, 2024
packages/components/src/custom-select-control-v2/legacy-component/test/index.tsx
Outdated
Show resolved
Hide resolved
ciampo
changed the title
CustomSelectControlV2: Fix selectedItem type in onChange callback for the legacy adapter
ciampo
deleted the
fix/custom-select-control-v2-legacy-adapter-option-handling
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow-up to/split from: #61272.
What?
Why
Before the changes here, the adapter built the object like this:
Source: https://github.com/WordPress/gutenberg/blob/trunk/packages/components/src/custom-select-control-v2/legacy-component/index.tsx#L46-L47
Notice the
selectedItemproperty there. There are a few things that are wrong with it.The classic component (V1) documents the
optionsas anArray<{ key: String, name: String, style: ?{}, className: ?String, ...rest }>(docs). Here we find the first (but less critical) smell in the adapter, theLegacyTypetype doesn't account for...rest:Source: https://github.com/WordPress/gutenberg/blob/trunk/packages/components/src/custom-select-control-v2/types.ts#L76.
It'd be easy to solve with an additional
[key: string]: anytype property there.The way the V2 legacy adapter is building the
selectedItemobject is wrong:keyandname. This is wrong as V1 can have an arbitrary number of properties for an item (see below). In addition to that,selectedItemfrom the font-appearance select looks like this when selected from V1:'{"key":"normal-200","name":"Extra Light","style":{"fontStyle":"normal","fontWeight":"200"}}'and from the adapter (without the fixes here):'{"name":"Extra Light","key":"Extra Light"}'.How?
The
optionsprop is the array of options passed by the consumer. It already has theLegacyProps, which is the right sub-type the callback param expects. So, instead of rebuilding the structure, we just passoptions:This fixes the contract with consumers by making the object passed to the callback follow the same interface as V1 (with the right structure).
There's still a problem of the selected item not being set to the current value when the element is mounted. This was already done in the parent PR and will be split in a subsequent PR.
Testing Instructions
For a quick way to test in the editor, you can apply the follow pach
Click to show the diff
And then in the Site Editor, open the Global Styles sidebar, click the "Typography" button, and then click any entry under the "Elements" group. In the new view that loads in the sidebar, notice how there are two identical "Appearance" dropdowns — the first one is implemented with the v1 component, the second one is implemented with the v2 legacy adapter.