Skip to content
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.

Sign up for GitHub

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

Jump to bottom

CircularOptionPicker: stop using composite store #64833

Merged
merged 4 commits into from
Sep 2, 2024
Merged

CircularOptionPicker: stop using composite store #64833

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7a76156
CircularOptionPicker: do not use composite store directly
ciampo Aug 27, 2024
aae1922
Wait for composite to initialize in unit tests
ciampo Aug 27, 2024
922607d
CHANGELOG
ciampo Aug 31, 2024
728336d
Merge branch 'trunk' into feat/stabilize-composite/refactor-circular-…
tyxla Sep 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions packages/block-editor/src/components/color-palette/test/control.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { render } from '@testing-library/react';
import { render, waitFor, queryByAttribute } from '@testing-library/react';

/**
* Internal dependencies
Expand All @@ -10,9 +10,22 @@ import ColorPaletteControl from '../control';

const noop = () => {};

async function renderAndValidate( ...renderArgs ) {
Copy link
Contributor Author

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 scale

Copy link
Member

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.

const view = render( ...renderArgs );
await waitFor( () => {
const activeButton = queryByAttribute(
'data-active-item',
view.baseElement,
'true'
);
expect( activeButton ).not.toBeNull();
} );
return view;
}

describe( 'ColorPaletteControl', () => {
it( 'matches the snapshot', async () => {
const { container } = render(
const { container } = await renderAndValidate(
<ColorPaletteControl
label="Test Color"
value="#f00"
Expand Down
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
- `UnitControl`
- `Modal`: Update animation effect ([#64580](https://github.com/WordPress/gutenberg/pull/64580)).
- `AlignmentMatrixControl`: do not use composite store directly ([#64850](https://github.com/WordPress/gutenberg/pull/64850)).
- `CircularOptionPicker`: do not use composite store directly ([#64833](https://github.com/WordPress/gutenberg/pull/64833)).

### Bug Fixes

Expand Down
35 changes: 16 additions & 19 deletions packages/components/src/circular-option-picker/circular-option-picker-option.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';

/**
Expand Down Expand Up @@ -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 );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#64723 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

@ciampo ciampo Sep 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ciampo ciampo Sep 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The 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 onFocusVisible ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 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

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

@diegohaz diegohaz Sep 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The 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 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.

ariakit/ariakit#4100

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for looking into it!

a better workaround is calling setActiveId from 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

}
}, [ isSelected, setActiveId, activeId, id ] );

return (
<Composite.Item
Expand All @@ -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'
Expand All @@ -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 } />
);
Expand Down
37 changes: 25 additions & 12 deletions packages/components/src/circular-option-picker/circular-option-picker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@ import clsx from 'clsx';
*/
import { useInstanceId } from '@wordpress/compose';
import { isRTL } from '@wordpress/i18n';
import { useMemo, useState } from '@wordpress/element';

/**
* Internal dependencies
*/
import { CircularOptionPickerContext } from './circular-option-picker-context';
import { Composite } from '../composite';
import { useCompositeStore } from '../composite/store';
import type {
CircularOptionPickerProps,
ListboxCircularOptionPickerProps,
Expand Down Expand Up @@ -86,24 +86,30 @@ function ListboxCircularOptionPicker(
...additionalProps
} = props;

const compositeStore = useCompositeStore( {
focusLoop: loop,
rtl: isRTL(),
} );
const [ activeId, setActiveId ] = useState< string | null | undefined >(
undefined
);

const compositeContext = {
baseId,
compositeStore,
};
const contextValue = useMemo(
() => ( {
baseId,
activeId,
setActiveId,
} ),
[ baseId, activeId, setActiveId ]
);

return (
<div className={ className }>
<CircularOptionPickerContext.Provider value={ compositeContext }>
<CircularOptionPickerContext.Provider value={ contextValue }>
<Composite
{ ...additionalProps }
id={ baseId }
store={ compositeStore }
focusLoop={ loop }
rtl={ isRTL() }
role="listbox"
activeId={ activeId }
setActiveId={ setActiveId }
>
{ options }
</Composite>
Expand All @@ -119,9 +125,16 @@ function ButtonsCircularOptionPicker(
) {
const { actions, options, children, baseId, ...additionalProps } = props;

const contextValue = useMemo(
() => ( {
baseId,
} ),
[ baseId ]
);

return (
<div { ...additionalProps } id={ baseId }>
<CircularOptionPickerContext.Provider value={ { baseId } }>
<CircularOptionPickerContext.Provider value={ contextValue }>
{ options }
{ children }
{ actions }
Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/circular-option-picker/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import type { Icon } from '@wordpress/icons';
import type { ButtonAsButtonProps } from '../button/types';
import type { DropdownProps } from '../dropdown/types';
import type { WordPressComponentProps } from '../context';
import type { Composite } from '../composite';

type CommonCircularOptionPickerProps = {
/**
Expand Down Expand Up @@ -125,5 +124,6 @@ export type OptionProps = Omit<

export type CircularOptionPickerContextProps = {
baseId?: string;
compositeStore?: React.ComponentProps< typeof Composite >[ 'store' ];
activeId?: string | null | undefined;
setActiveId?: ( newId: string | null | undefined ) => void;
};
Loading