Components: Replace/old custom select control with v2 legacy adapter #61272
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
commented
May 1, 2024
| const changeObject = { | ||
| highlightedIndex: state.renderedItems.findIndex( | ||
| ( item ) => item.value === nextValue | ||
| ), | ||
| inputValue: '', | ||
| isOpen: state.open, | ||
| selectedItem: { |
There was a problem hiding this comment.
The old object here didn't include any custom attributes that might was part of the original options array passed to the component. Not sure we should instead pass these values through the Ariakit's API (e.g use value to store the style data for example), but selecting the option by name is a (hacky) workaround that works for now.
There was a problem hiding this comment.
OK, after spending more time with the codebase for V2/LegacyAdapter and the V1 component, I think I grasped what was wrong here. There were two issues:
- The
selectedItemobject had itsnameandkeyboth set to thenextValueargument passed to thesetValuecallback, resulting in both having thenameattribute from the options array. This didn't look right. - The same structure was missing the style metadata (
styleand/orclassName) that was considered to be implicitly available in the item passed to theonChangecallback in V1, which some consumers expect.
I've fixed both in 56461bd, but I'm wondering if this is the right approach and if should we keep the same API in V2 (as in keep the style metadata in the item passed to onChange?), or should we refactor the consumers to not rely on this self-contained data and instead move it to a store somewhere else? Of course, this would be work to be done in follow-up PRs.
See the test / comment here (I'll simplify or remove that comment before merging).
|
Size Change: +4.71 kB (+0.27%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
fullofcaffeine
force-pushed
the
replace/old-custom-select-control-with-v2-legacy-adapter
branch
from
92b652d
to
f255b84
Compare
fullofcaffeine
force-pushed
the
replace/old-custom-select-control-with-v2-legacy-adapter
branch
from
f255b84
to
e019e2f
Compare
fullofcaffeine
commented
May 10, 2024
| @@ -286,7 +286,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.
The fact it had the same key and name looked like a bug caused by the previous logic here.
fullofcaffeine
force-pushed
the
replace/old-custom-select-control-with-v2-legacy-adapter
branch
from
e019e2f
to
56461bd
Compare
|
After addressing this, I tested both V1 and V2 through the adapter manually via the Typography Panel's "Appearance" select, which uses the custom select UI control. I've so far found a few styling bugs + one behavior bug that need to be addressed:
The Leacy control (or V2) has a Then, if you select an item, the style is correctly applied to the paragraph, but if you move the focus to another paragraph and then go back to the one you just applied the style, the style resets because the selection will render with the default item selected. I'll be looking into those issues in the following days. |
fullofcaffeine
force-pushed
the
replace/old-custom-select-control-with-v2-legacy-adapter
branch
3 times, most recently
from
b7dd66f
to
387ad3b
Compare
Fixed in 387ad3b, pending the introduction of a regression test. I'll look into it + the z-index CSS issue tomorrow. |
fullofcaffeine
commented
May 15, 2024
packages/block-editor/src/components/font-appearance-control/index.js
Outdated
Show resolved
Hide resolved
fullofcaffeine
force-pushed
the
replace/old-custom-select-control-with-v2-legacy-adapter
branch
from
56b5c52
to
0491d42
Compare
I fixed the most pressing visual bugs: Screen.Recording.2024-05-15.at.8.34.11.p.m.movHowever, there are still a few slight differences (which you can see in the screencast above):
I think the next step would be to describe those issues as stories in Storybook and handle them there (e.g add a new story that renders the select with a lot of components, then figure out how it should behave - try the same in the legacy one and decide on a compromise). I'll also look into other consumers and see if I can find other inconsistencies. |
fullofcaffeine
force-pushed
the
replace/old-custom-select-control-with-v2-legacy-adapter
branch
3 times, most recently
from
a17cb3a
to
7e6311d
Compare
|
I think this is ready for a review pass. Here are a few consumers that can be used to test it in practice:
There's another one under https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/hooks/position.js#L290, but I couldn't find exactly where this feature is yet. There might be other slight visual changes, and the review's goal is also to discuss if it's okay to keep them or if we should fix them (as in making it exactly/closer to V1). |
|
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. |
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
fullofcaffeine
force-pushed
the
replace/old-custom-select-control-with-v2-legacy-adapter
branch
from
213389c
to
d3f128d
Compare
…dapter and fix selectedItem handling `onChange`
…t the option with custom property scenario
…mount + fix other tests
…e display:block or it might cause visual inconsistencies with other display modes (like flex)
This was making the l-beam cursor appear when hovering over the items, which is distracting when you want to just select an item for the select.
fullofcaffeine
force-pushed
the
replace/old-custom-select-control-with-v2-legacy-adapter
branch
from
8cb8f2c
to
c757eb0
Compare
|
Flaky tests detected in c757eb0. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9166808983
|
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.
What?
CustomSelectControlwith the legacy adapter forCustomSelectControlV2;Why?
The new
CustomSelectControlV2has been recently introduced in the library and provides new accessibility features. While we don't migrate to V2, the legacy adapter provides an API adapter that should make the migration easier and more gradual. This PR is an attempt to do that, and also fix any bugs that we might find along the way.How?
By replacing the old 'CustomSelectControl' export with the new implementation (the legacy adapter for V2). Also, testing consumers and seeing if the new implementation works well, and fixing bugs along the way (might split into other PRs later).
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast