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

Dataviews Filter search widget: do not use Composite store #64985

Merged
merged 6 commits into from
Sep 3, 2024
Merged

Dataviews Filter search widget: do not use Composite store #64985

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c2a7eca
Dataviews Filter search widget: do not use Composite store
ciampo Sep 2, 2024
58e689a
Use internal CompositeHover and CompositeTypeahead version
ciampo Sep 2, 2024
7547cdb
Better comment
ciampo Sep 2, 2024
c15399a
Refactor generateCompositeItemId arguments
ciampo Sep 3, 2024
0dc1866
Export Composite.Typeahead and Composite.Hover as private APIs
ciampo Sep 3, 2024
4b9d7c8
CHANGELOG
ciampo Sep 3, 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
2 changes: 2 additions & 0 deletions packages/components/src/private-apis.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ lock( privateApis, {
CompositeGroupV2: Composite.Group,
CompositeItemV2: Composite.Item,
CompositeRowV2: Composite.Row,
CompositeTypeaheadV2: Composite.Typeahead,
CompositeHoverV2: Composite.Hover,
ciampo marked this conversation as resolved.
Show resolved Hide resolved
useCompositeStoreV2: useCompositeStore,
__experimentalPopoverLegacyPositionToPlacement,
createPrivateSlotFill,
Expand Down
60 changes: 44 additions & 16 deletions packages/dataviews/src/components/dataviews-filters/search-widget.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import removeAccents from 'remove-accents';
/**
* WordPress dependencies
*/
import { useInstanceId } from '@wordpress/compose';
import { __, sprintf } from '@wordpress/i18n';
import { useState, useMemo, useDeferredValue } from '@wordpress/element';
import {
Expand All @@ -27,7 +28,8 @@ import type { Filter, NormalizedFilter, View } from '../../types';
const {
CompositeV2: Composite,
CompositeItemV2: CompositeItem,
useCompositeStoreV2: useCompositeStore,
CompositeHoverV2: CompositeHover,
CompositeTypeaheadV2: CompositeTypeahead,
} = unlock( componentsPrivateApis );

interface SearchWidgetProps {
Expand Down Expand Up @@ -84,22 +86,37 @@ const getNewValue = (
return [ value ];
};

function generateFilterElementCompositeItemId(
prefix: string,
filterElementValue: string
) {
return `${ prefix }-${ filterElementValue }`;
}

function ListBox( { view, filter, onChangeView }: SearchWidgetProps ) {
const compositeStore = useCompositeStore( {
virtualFocus: true,
focusLoop: true,
// When we have no or just one operator, we can set the first item as active.
// We do that by passing `undefined` to `defaultActiveId`. Otherwise, we set it to `null`,
// so the first item is not selected, since the focus is on the operators control.
defaultActiveId: filter.operators?.length === 1 ? undefined : null,
} );
const baseId = useInstanceId( ListBox, 'dataviews-filter-list-box' );

const [ activeCompositeId, setActiveCompositeId ] = useState<
string | null | undefined
>(
// When there are one or less operators, the first item is set as active
// (by setting the initial `activeId` to `undefined`).
// With 2 or more operators, the focus is moved on the operators control
// (by setting the initial `activeId` to `null`), meaning that there won't
// be an active item initially. Focus is then managed via the
// `onFocusVisible` callback.
filter.operators?.length === 1 ? undefined : null
);
const currentFilter = view.filters?.find(
( f ) => f.field === filter.field
);
const currentValue = getCurrentValue( filter, currentFilter );
return (
<Composite
store={ compositeStore }
virtualFocus
focusLoop
activeId={ activeCompositeId }
Copy link
Member

Choose a reason for hiding this comment

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

We were previously working with defaultActiveId and not activeId. Are we certain this won't introduce suble differences?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are using to activeId instead of defaultActiveId since we are switching from using Composite uncontrolled, to controlling its active item ID state.

I moved the defaultActiveId value to the initial state value for activeCompositeId.

I'll look more into it, and report any findings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This logic affects the composite widget when opening the dropdown — it basically prevents Composite from automatically picking an active composite item if there is more than one filter operator.

I reviewed the code, and I don't think this change will introduce differences in the current state of the code since the filter.operators array doesn't seem to change while the dialog is open.

If this assumption was ever going to change, I suspect the previous version would have also incurred in some unexpected behaviour

Copy link
Contributor Author

@ciampo ciampo Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In general, the current implementation feels a bit hacky. I wish we didn't have to control the activeId state at all. With more time and and a willingness to tweak this UI a bit, we could also consider having different styles for active vs focused items.

But not in the scope for this PR, and without a high priority.

setActiveId={ setActiveCompositeId }
role="listbox"
className="dataviews-filters__search-widget-listbox"
aria-label={ sprintf(
Expand All @@ -108,18 +125,29 @@ function ListBox( { view, filter, onChangeView }: SearchWidgetProps ) {
filter.name
) }
onFocusVisible={ () => {
if ( ! compositeStore.getState().activeId ) {
compositeStore.move( compositeStore.first() );
// `onFocusVisible` needs the `Composite` component to be focusable,
// which is implicitly achieved via the `virtualFocus: true` option
// in the `useCompositeStore` hook.
if ( ! activeCompositeId && filter.elements.length ) {
setActiveCompositeId(
generateFilterElementCompositeItemId(
baseId,
filter.elements[ 0 ].value
)
);
}
} }
render={ <Ariakit.CompositeTypeahead store={ compositeStore } /> }
render={ <CompositeTypeahead /> }
>
{ filter.elements.map( ( element ) => (
<Ariakit.CompositeHover
store={ compositeStore }
<CompositeHover
key={ element.value }
render={
<CompositeItem
id={ generateFilterElementCompositeItemId(
baseId,
element.value
) }
render={
<div
aria-label={ element.label }
Expand Down Expand Up @@ -185,7 +213,7 @@ function ListBox( { view, filter, onChangeView }: SearchWidgetProps ) {
) }
</span>
<span>{ element.label }</span>
</Ariakit.CompositeHover>
</CompositeHover>
) ) }
</Composite>
);
Expand Down
Loading