-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
CircularOptionPicker: stop using composite store #64833
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,14 +2,13 @@ | |
* External dependencies | ||
*/ | ||
import clsx from 'clsx'; | ||
import { useStoreState } from '@ariakit/react'; | ||
import type { ForwardedRef } from 'react'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { useInstanceId } from '@wordpress/compose'; | ||
import { forwardRef, useContext } from '@wordpress/element'; | ||
import { forwardRef, useContext, useEffect } from '@wordpress/element'; | ||
import { Icon, check } from '@wordpress/icons'; | ||
|
||
/** | ||
|
@@ -46,18 +45,21 @@ function UnforwardedOptionAsOption( | |
id: string; | ||
className?: string; | ||
isSelected?: boolean; | ||
compositeStore: NonNullable< | ||
React.ComponentProps< typeof Composite >[ 'store' ] | ||
>; | ||
}, | ||
forwardedRef: ForwardedRef< any > | ||
) { | ||
const { id, isSelected, compositeStore, ...additionalProps } = props; | ||
const activeId = useStoreState( compositeStore, 'activeId' ); | ||
const { id, isSelected, ...additionalProps } = props; | ||
|
||
if ( isSelected && ! activeId ) { | ||
compositeStore.setActiveId( id ); | ||
} | ||
const { setActiveId, activeId } = useContext( CircularOptionPickerContext ); | ||
|
||
useEffect( () => { | ||
if ( isSelected && ! activeId ) { | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey @diegohaz , do you have any advice better alternatives to using @tyxla suggested using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 With the changes from this PR, though, our call to
In short, it seems so. That's why we're discussing about the timing of the call. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An alternative that came to mind — would it be better to set the active id in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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:
@diegohaz hope the context is a bit more clear around what we're trying to achieve here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this looks good to go. We can always iterate if @diegohaz has a better suggestion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried to reproduce this with Ariakit, but I might be missing something. There's no need for 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 In this case, a better workaround is calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for looking into it!
The main reason for this refactor is to avoid using the store directly from outside the |
||
} | ||
}, [ isSelected, setActiveId, activeId, id ] ); | ||
|
||
return ( | ||
<Composite.Item | ||
|
@@ -83,9 +85,7 @@ export function Option( { | |
tooltipText, | ||
...additionalProps | ||
}: OptionProps ) { | ||
const { baseId, compositeStore } = useContext( | ||
CircularOptionPickerContext | ||
); | ||
const { baseId, setActiveId } = useContext( CircularOptionPickerContext ); | ||
const id = useInstanceId( | ||
Option, | ||
baseId || 'components-circular-option-picker__option' | ||
|
@@ -97,12 +97,9 @@ export function Option( { | |
...additionalProps, | ||
}; | ||
|
||
const optionControl = compositeStore ? ( | ||
<OptionAsOption | ||
{ ...commonProps } | ||
compositeStore={ compositeStore } | ||
isSelected={ isSelected } | ||
/> | ||
const isListbox = setActiveId !== undefined; | ||
const optionControl = isListbox ? ( | ||
<OptionAsOption { ...commonProps } isSelected={ isSelected } /> | ||
) : ( | ||
<OptionAsButton { ...commonProps } isPressed={ isSelected } /> | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 scaleThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for this particular instance. Since this is a testing detail, let's worry about it if it actually needs to scale.