-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Dataviews Filter search widget: do not use Composite store #64985
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
Changes from all commits
c2a7eca
58e689a
7547cdb
c15399a
0dc1866
4b9d7c8
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 |
|---|---|---|
|
|
@@ -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 { | ||
|
|
@@ -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 { | ||
|
|
@@ -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 } | ||
|
Member
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. We were previously working with
Contributor
Author
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. We are using to I moved the I'll look more into it, and report any findings
Contributor
Author
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. This logic affects the composite widget when opening the dropdown — it basically prevents I reviewed the code, and I don't think this change will introduce differences in the current state of the code since the If this assumption was ever going to change, I suspect the previous version would have also incurred in some unexpected behaviour
Contributor
Author
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. In general, the current implementation feels a bit hacky. I wish we didn't have to control the 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( | ||
|
|
@@ -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 } | ||
|
|
@@ -185,7 +213,7 @@ function ListBox( { view, filter, onChangeView }: SearchWidgetProps ) { | |
| ) } | ||
| </span> | ||
| <span>{ element.label }</span> | ||
| </Ariakit.CompositeHover> | ||
| </CompositeHover> | ||
| ) ) } | ||
| </Composite> | ||
| ); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.