Components: Replace/old custom select control with v2 legacy adapter #61272
Conversation
| 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
| @@ -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. |
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
2 times, most recently
from
8cb8f2c to
c757eb0
Compare
|
Flaky tests detected in c757eb0. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9166808983
|
…dapter and fix selectedItem handling `onChange`
…t the option with custom property scenario
…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
c757eb0 to
cd537af
Compare
fullofcaffeine
force-pushed
the
replace/old-custom-select-control-with-v2-legacy-adapter
branch
3 times, most recently
from
24f2bd8 to
cd537af
Compare
|
I've merged #60261 here. After fixing a few conflicts, functionality-wise, the control is working fine (with the fixes in this PR). I did find a few visual inconsistencies, though, including one that is not related to the Let's start out by comparing the components visually before I merge the changeset here and after. I recorded a screencast both in Storybook and in the editor for both scenarios. Before the
|
| Storybook | Editor |
|---|---|
beforemerging-storybook.mov |
beforemerging-editor.mov |
This is the baseline on which I attempted to fix most of the visual issues that existed when the controls were within the editor's constraints.
After merging the InputBase refactor PR
| Storybook | Editor |
|---|---|
aftermerging-storybook.mov |
editor-aftermerging.mov |
Here, I found a visual glitch that happens when the selected item label text is long enough to be broken into two lines and the select width is small -- there's an additional left padding (or not enough right padding, depending on how you look at it) making it look unbalanced visually. In the refactored version, you can also see that there's a slight difference in the bottom padding for the SelectLabel ("Appearance") in the InputBase version—you can ignore that as I already fixed it.
Here's a comparison:
Size = 'default'
After InputBase |
Before |
|---|---|
 |
 |
To try to fix it in the InputBase version, I've played with the padding here and increasing the paddingInlineEnd + 2 seems to make it a bit better, but not sure if this is the right/best approach:
This:
default: {
[ heightProperty ]: 40,
paddingInlineStart: space( 2 ),
paddingInlineEnd: space( 6 ),
},Renders it like this:
Which looks close enough to how it was before the InputBase refactor.
Size = 'compact'
After InputBase |
Before |
|---|---|
 |
 |
The solution used for the default size works the same here, same padding values.
Size = 'small'
This difference is not as evident in this case, though there is some different padding for the chevron it seems:
After InputBase |
Before |
|---|---|
 |
 |
In this case, we could perhaps just leave it as is since the difference could be considered negligible?
Other issues
There's also another visual glitch that happens in both versions, when the selected text is too large, it just overflows the select boundaries:
This also happens in Storybook if the viewport is small enough, but we probably didn't notice because the size of the control must be quite small for an overflow to occur.
Should we handle this somehow? Maybe we should force the select to be as wide as the biggest label it has by default? This could be an interesting improvement to explore in a follow-up.
These are the visual glitches I've noticed. I haven't noticed anything else (major) other than that.
|
Thank you for all your work here! I took a look at all the issues addressed in this PR, and each of them are big enough to be reviewed and discussed in separate PRs. Specifically:
As for the long strings issue you summarized in the last update, this was an issue I punted from the InputBase PR (#60261) because it looked like I might want to refactor things from the |
|
@mirka Cool! Thanks for the feedback! I'll start splitting into separate PRs then 👍🏻 |
Ah, copy that. I did add a unit test but in the legacy adapter suite. If my memory doesn't fail, this wasn't explicitly tested in V1 at all; I'll add that as part of the split PR for 1). |
Addressed in #62198. |
|
Closing this in favor of splitting into smaller PRs. See linked PRs for more details. |
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