CircularOptionPicker: stop using composite store #64833
Merged
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
|
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. |
ciampo
force-pushed
the
feat/stabilize-composite/refactor-circular-option-picker
branch
from
f124cf0 to
d9b9f8d
Compare
ciampo
changed the title
ciampo
added
[Type] Enhancement
ciampo
force-pushed
the
feat/stabilize-composite/refactor-circular-option-picker
branch
from
d9b9f8d to
d2a9da3
Compare
|
Size Change: +7 B (0%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 386c63f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10581481840
|
ciampo
force-pushed
the
feat/stabilize-composite/refactor-circular-option-picker
branch
from
386c63f to
292c94c
Compare
ciampo
commented
Aug 28, 2024
| // The setTimeout call is necessary to make sure that this update | ||
| // doesn't get overridden by `Composite`'s internal logic, which picks | ||
| // an initial active item if one is not specifically set. | ||
| window.setTimeout( () => setActiveId?.( id ), 0 ); |
There was a problem hiding this comment.
Hey @diegohaz , do you have any advice better alternatives to using setTimeout here? Without it, it looks like Composite's internal logic to pick an initially selected item overrides this selection.
@tyxla suggested using queueMicrotask, while @mirka suggesting calling setItems instead. (see link above). What's your take? Is this behaviour expected? Is there a more elegant way to make this work?
There was a problem hiding this comment.
After reading the comment and the discussion you linked, I still don't understand its purpose. Could you clarify?
Does Ariakit still set an active id even after you set one yourself?
There was a problem hiding this comment.
CircularOptionPicker displays a few color options as a composite widget.
If the component loads and then a color option is selected (programmatically), then we'd like to set that color option as the active composite item, so that when a user moves its focus on the composite widget, the selected color (and not the first color in the list) gets focus.
Previously, the component was calling compositeStore.setActiveId( id ) insider the render function (not wrapped in useEffect), and that seemed to work.
With the changes from this PR, though, our call to setActiveId gets almost immediately overridden by some internal ariakit logic which sets the active item to the first item after out setActiveId call.
Does Ariakit still set an active id even after you set one yourself?
In short, it seems so. That's why we're discussing about the timing of the call.
There was a problem hiding this comment.
An alternative that came to mind — would it be better to set the active id in onFocusVisible ?
There was a problem hiding this comment.
An alternative that came to mind — would it be better to set the active id in
onFocusVisible?
I looked into it, and it would make the code even more convoluted, since it would cause the composite wrapper to also become focusable.
What we need is:
- if the user hasn't interacted yet with the composite widget AND there is a selected option, that option should become the active item — so that focus moves to that option when the user first focuses the widget;
- in any other scenario, the first item should be the active item
@diegohaz hope the context is a bit more clear around what we're trying to achieve here
There was a problem hiding this comment.
I think this looks good to go. We can always iterate if @diegohaz has a better suggestion.
There was a problem hiding this comment.
I tried to reproduce this with Ariakit, but I might be missing something. There's no need for setTimeout here: https://stackblitz.com/edit/cnsby2?file=command%2Findex.tsx&theme=dark
Can you try to reproduce it from that code? This could be a bug.
Edit: I can reproduce it if I pass down an external setActiveId instead of calling setActiveId from the composite store: https://stackblitz.com/edit/cnsby2-kp1gfx?file=command%2Findex.tsx&theme=dark
In this case, a better workaround is calling setActiveId from the Ariakit composite store if you have access to it.
There was a problem hiding this comment.
Thank you for looking into it!
a better workaround is calling
setActiveIdfrom the Ariakit composite store if you have access to it.
The main reason for this refactor is to avoid using the store directly from outside the Composite component, therefore we're probably going to wait for ariakit/ariakit#4100
ciampo
force-pushed
the
feat/stabilize-composite/refactor-circular-option-picker
branch
2 times, most recently
from
49d37d1 to
e98dd7b
Compare
ciampo
commented
Sep 2, 2024
| @@ -10,9 +10,22 @@ import ColorPaletteControl from '../control'; | |||
|
|
|||
| const noop = () => {}; | |||
|
|
|||
| async function renderAndValidate( ...renderArgs ) { | |||
There was a problem hiding this comment.
Not sure what we ended up deciding re. the render function — for now, I'm using a composite-specific workaround, but I'm not sure how well this would scale
There was a problem hiding this comment.
Looks good for this particular instance. Since this is a testing detail, let's worry about it if it actually needs to scale.
ciampo
force-pushed
the
feat/stabilize-composite/refactor-circular-option-picker
branch
from
e98dd7b to
922607d
Compare
tyxla
approved these changes
Sep 2, 2024
ciampo
deleted the
feat/stabilize-composite/refactor-circular-option-picker
branch
2 tasks
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?
Requires #64832 to be merged first
Extracted from #64723
Refactor
CircularOptionPickerso that it doesn't use thestorefromuseCompositeStoreto function.Why?
See #63704 (comment)
How?
By controlling the
activeIdstate via props.Testing Instructions
Make sure that existing usages of
CircularOptionPickercontinue to work as expected.In particular, make sure that, when
CircularOptionPickerhas a selected option (and the selected option is not the first option), tabbing on the component for the first time will automatically focus the selected option, instead of the first option.This can be tested in Storybook by applying the following diff — in the default example, the second item (green) should now be initially selected