From adbde7a5833b6d7c16c8ac581fa569785a15abf5 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Mon, 2 Sep 2024 18:38:57 +0200 Subject: [PATCH 01/10] Move over changes from POW PR --- .../src/dataviews-layouts/list/index.tsx | 173 ++++++++++++------ 1 file changed, 116 insertions(+), 57 deletions(-) diff --git a/packages/dataviews/src/dataviews-layouts/list/index.tsx b/packages/dataviews/src/dataviews-layouts/list/index.tsx index d8889f25b24e9c..897f7759ce4e89 100644 --- a/packages/dataviews/src/dataviews-layouts/list/index.tsx +++ b/packages/dataviews/src/dataviews-layouts/list/index.tsx @@ -2,17 +2,11 @@ * External dependencies */ import clsx from 'clsx'; -// TODO: use the @wordpress/components one once public -// eslint-disable-next-line no-restricted-imports -import { useStoreState } from '@ariakit/react'; -// Import CompositeStore type, which is not exported from @wordpress/components. -// eslint-disable-next-line no-restricted-imports -import type { CompositeStore } from '@ariakit/react'; /** * WordPress dependencies */ -import { useInstanceId } from '@wordpress/compose'; +import { useInstanceId, usePrevious } from '@wordpress/compose'; import { __experimentalHStack as HStack, __experimentalVStack as VStack, @@ -44,24 +38,28 @@ import type { Action, NormalizedField, ViewListProps } from '../../types'; interface ListViewItemProps< Item > { actions: Action< Item >[]; - id?: string; + id: string; isSelected: boolean; item: Item; mediaField?: NormalizedField< Item >; onSelect: ( item: Item ) => void; primaryField?: NormalizedField< Item >; - store: CompositeStore; visibleFields: NormalizedField< Item >[]; + selectPreviousItemDropdownTrigger: () => void; + selectNextItemDropdownTrigger: () => void; } const { - useCompositeStoreV2: useCompositeStore, CompositeV2: Composite, CompositeItemV2: CompositeItem, CompositeRowV2: CompositeRow, DropdownMenuV2: DropdownMenu, } = unlock( componentsPrivateApis ); +function generateDropdownTriggerCompositeId( domId: string ) { + return `${ domId }-dropdown`; +} + function ListItem< Item >( { actions, id, @@ -70,8 +68,9 @@ function ListItem< Item >( { mediaField, onSelect, primaryField, - store, visibleFields, + selectPreviousItemDropdownTrigger, + selectNextItemDropdownTrigger, }: ListViewItemProps< Item > ) { const registry = useRegistry(); const itemRef = useRef< HTMLElement >( null ); @@ -147,7 +146,6 @@ function ListItem< Item >( { >
} role="button" id={ id } @@ -213,7 +211,7 @@ function ListItem< Item >( { { primaryAction && 'RenderModal' in primaryAction && (
( { ! ( 'RenderModal' in primaryAction ) && (
( { ( { label={ __( 'Actions' ) } accessibleWhenDisabled disabled={ ! actions.length } - onKeyDown={ ( event: { - key: string; - preventDefault: () => void; - } ) => { + // Prevent the default behavior (open dropdown menu) + // and instead move the composite item selection. + // https://github.com/ariakit/ariakit/issues/3768 + onKeyDown={ ( + event: React.KeyboardEvent< HTMLButtonElement > + ) => { if ( event.key === 'ArrowDown' ) { - // Prevent the default behaviour (open dropdown menu) and go down. event.preventDefault(); - store.move( - store.down() - ); + selectNextItemDropdownTrigger(); } if ( event.key === 'ArrowUp' ) { - // Prevent the default behavior (open dropdown menu) and go up. event.preventDefault(); - store.move( - store.up() - ); + selectPreviousItemDropdownTrigger(); } } } /> @@ -331,10 +327,32 @@ export default function ViewList< Item >( props: ViewListProps< Item > ) { view, } = props; const baseId = useInstanceId( ViewList, 'view-list' ); + + const getItemDomId = useCallback( + ( item?: Item ) => { + if ( ! item ) { + return; + } + + return `${ baseId }-${ getItemId( item ) }`; + }, + [ baseId, getItemId ] + ); + const selectedItem = data?.findLast( ( item ) => selection.includes( getItemId( item ) ) ); + const onSelect = ( item: Item ) => + onChangeSelection( [ getItemId( item ) ] ); + + const [ activeCompositeId, setActiveCompositeId ] = useState< + string | null | undefined + >( + // By default, the active composite item is the selected one. + getItemDomId( selectedItem ) + ); + const mediaField = fields.find( ( field ) => field.id === view.layout?.mediaField ); @@ -350,37 +368,70 @@ export default function ViewList< Item >( props: ViewListProps< Item > ) { ) ); - const onSelect = ( item: Item ) => - onChangeSelection( [ getItemId( item ) ] ); - - const getItemDomId = useCallback( - ( item?: Item ) => - item ? `${ baseId }-${ getItemId( item ) }` : undefined, - [ baseId, getItemId ] - ); - - const store = useCompositeStore( { - defaultActiveId: getItemDomId( selectedItem ), - } ) as CompositeStore; // TODO, remove once composite APIs are public + const activeItemIndex = data.findIndex( ( item ) => { + const itemCompositeIdPrefix = getItemDomId( item ); + return ( + !! itemCompositeIdPrefix && + activeCompositeId?.startsWith( itemCompositeIdPrefix ) + ); + } ); + const previousActiveItemIndex = usePrevious( activeItemIndex ); + const isActiveIdInList = activeItemIndex !== -1; - // Manage focused item, when the active one is removed from the list. - const isActiveIdInList = useStoreState( - store, - ( state: { items: any[]; activeId: any } ) => - state.items.some( - ( item: { id: any } ) => item.id === state.activeId - ) - ); useEffect( () => { if ( ! isActiveIdInList ) { - // Prefer going down, except if there is no item below (last item), then go up (last item in list). - if ( store.down() ) { - store.move( store.down() ); - } else if ( store.up() ) { - store.move( store.up() ); - } + // Prefer going down, except if there is no item below (last item), + // then go up (last item in list). + // By picking previousActiveItemIndex as the next item index, we are + // basically picking the item after the deleted one. + // Clamping between 0 and data.length - 1 to avoid out of bounds. + const nextActiveDataItem = + data[ + Math.max( + 0, + Math.min( + previousActiveItemIndex ?? 0, + data.length - 1 + ) + ) + ]; + + const nextCompositeActiveId = getItemDomId( nextActiveDataItem ); + setActiveCompositeId( nextCompositeActiveId ); + ( + document.querySelector( `#${ nextCompositeActiveId }` ) as + | HTMLElement + | undefined + )?.focus(); } - }, [ isActiveIdInList ] ); + }, [ data, previousActiveItemIndex, getItemDomId, isActiveIdInList ] ); + + const selectItemDropdownTrigger = ( itemIndex: number ) => { + const item = data[ itemIndex ]; + const itemDomId = getItemDomId( item ); + if ( ! itemDomId ) { + return; + } + const nextCompositeActiveId = + generateDropdownTriggerCompositeId( itemDomId ); + setActiveCompositeId( nextCompositeActiveId ); + ( + document.querySelector( `#${ nextCompositeActiveId }` ) as + | HTMLElement + | undefined + )?.focus(); + }; + + const selectPreviousItemDropdownTrigger = () => { + selectItemDropdownTrigger( + Math.min( data.length - 1, Math.max( 0, activeItemIndex - 1 ) ) + ); + }; + const selectNextItemDropdownTrigger = () => { + selectItemDropdownTrigger( + Math.min( data.length - 1, Math.max( 0, activeItemIndex + 1 ) ) + ); + }; const hasData = data?.length; if ( ! hasData ) { @@ -404,10 +455,13 @@ export default function ViewList< Item >( props: ViewListProps< Item > ) { render={
    } className="dataviews-view-list" role="grid" - store={ store } + activeId={ activeCompositeId } + setActiveId={ setActiveCompositeId } > { data.map( ( item ) => { - const id = getItemDomId( item ); + // Since `item` is guaranteed not to be undefined, we can safely + // let typescript know that `id` is not undefined either. + const id = getItemDomId( item )!; return ( ( props: ViewListProps< Item > ) { onSelect={ onSelect } mediaField={ mediaField } primaryField={ primaryField } - store={ store } visibleFields={ visibleFields } + selectPreviousItemDropdownTrigger={ + selectPreviousItemDropdownTrigger + } + selectNextItemDropdownTrigger={ + selectNextItemDropdownTrigger + } /> ); } ) } From 6903275fe96db09b48f3ed1d7eb93988f78a5331 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Mon, 2 Sep 2024 18:46:39 +0200 Subject: [PATCH 02/10] Simpler new active item comment --- .../src/dataviews-layouts/list/index.tsx | 23 ++++++++----------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/packages/dataviews/src/dataviews-layouts/list/index.tsx b/packages/dataviews/src/dataviews-layouts/list/index.tsx index 897f7759ce4e89..17c0261b13bb7b 100644 --- a/packages/dataviews/src/dataviews-layouts/list/index.tsx +++ b/packages/dataviews/src/dataviews-layouts/list/index.tsx @@ -380,21 +380,16 @@ export default function ViewList< Item >( props: ViewListProps< Item > ) { useEffect( () => { if ( ! isActiveIdInList ) { - // Prefer going down, except if there is no item below (last item), - // then go up (last item in list). - // By picking previousActiveItemIndex as the next item index, we are - // basically picking the item after the deleted one. + // By picking `previousActiveItemIndex` as the next item index, we are + // basically picking the item that would have been after the deleted one. + // If the previously active (and removed) item was the last of the list, + // we will select the item before it — which is the new last item. // Clamping between 0 and data.length - 1 to avoid out of bounds. - const nextActiveDataItem = - data[ - Math.max( - 0, - Math.min( - previousActiveItemIndex ?? 0, - data.length - 1 - ) - ) - ]; + const clampedPreviousActiveItemIndex = Math.max( + 0, + Math.min( previousActiveItemIndex ?? 0, data.length - 1 ) + ); + const nextActiveDataItem = data[ clampedPreviousActiveItemIndex ]; const nextCompositeActiveId = getItemDomId( nextActiveDataItem ); setActiveCompositeId( nextCompositeActiveId ); From 96f8e2c12e11830fd0540257ef7659529dd3e32f Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Tue, 3 Sep 2024 09:37:30 +0200 Subject: [PATCH 03/10] Simplify dropdown trigger keydown override --- .../src/dataviews-layouts/list/index.tsx | 103 ++++++++---------- 1 file changed, 48 insertions(+), 55 deletions(-) diff --git a/packages/dataviews/src/dataviews-layouts/list/index.tsx b/packages/dataviews/src/dataviews-layouts/list/index.tsx index 17c0261b13bb7b..eecff5a7604714 100644 --- a/packages/dataviews/src/dataviews-layouts/list/index.tsx +++ b/packages/dataviews/src/dataviews-layouts/list/index.tsx @@ -45,8 +45,7 @@ interface ListViewItemProps< Item > { onSelect: ( item: Item ) => void; primaryField?: NormalizedField< Item >; visibleFields: NormalizedField< Item >[]; - selectPreviousItemDropdownTrigger: () => void; - selectNextItemDropdownTrigger: () => void; + onDropdownTriggerKeyDown: React.KeyboardEventHandler< HTMLButtonElement >; } const { @@ -69,8 +68,7 @@ function ListItem< Item >( { onSelect, primaryField, visibleFields, - selectPreviousItemDropdownTrigger, - selectNextItemDropdownTrigger, + onDropdownTriggerKeyDown, }: ListViewItemProps< Item > ) { const registry = useRegistry(); const itemRef = useRef< HTMLElement >( null ); @@ -276,26 +274,9 @@ function ListItem< Item >( { label={ __( 'Actions' ) } accessibleWhenDisabled disabled={ ! actions.length } - // Prevent the default behavior (open dropdown menu) - // and instead move the composite item selection. - // https://github.com/ariakit/ariakit/issues/3768 - onKeyDown={ ( - event: React.KeyboardEvent< HTMLButtonElement > - ) => { - if ( - event.key === - 'ArrowDown' - ) { - event.preventDefault(); - selectNextItemDropdownTrigger(); - } - if ( - event.key === 'ArrowUp' - ) { - event.preventDefault(); - selectPreviousItemDropdownTrigger(); - } - } } + onKeyDown={ + onDropdownTriggerKeyDown + } /> } /> @@ -326,6 +307,7 @@ export default function ViewList< Item >( props: ViewListProps< Item > ) { selection, view, } = props; + const baseId = useInstanceId( ViewList, 'view-list' ); const getItemDomId = useCallback( @@ -401,32 +383,48 @@ export default function ViewList< Item >( props: ViewListProps< Item > ) { } }, [ data, previousActiveItemIndex, getItemDomId, isActiveIdInList ] ); - const selectItemDropdownTrigger = ( itemIndex: number ) => { - const item = data[ itemIndex ]; - const itemDomId = getItemDomId( item ); - if ( ! itemDomId ) { - return; - } - const nextCompositeActiveId = - generateDropdownTriggerCompositeId( itemDomId ); - setActiveCompositeId( nextCompositeActiveId ); - ( - document.querySelector( `#${ nextCompositeActiveId }` ) as - | HTMLElement - | undefined - )?.focus(); - }; + // Prevent the default behavior (open dropdown menu) and instead select the + // dropdown menu trigger on the previous/next row. + // https://github.com/ariakit/ariakit/issues/3768 + const onDropdownTriggerKeyDown = useCallback( + ( event: React.KeyboardEvent< HTMLButtonElement > ) => { + function selectItemDropdownTrigger( itemIndex: number ) { + const item = data[ itemIndex ]; + const itemDomId = getItemDomId( item ); + if ( ! itemDomId ) { + return; + } + const targetCompositeActiveId = + generateDropdownTriggerCompositeId( itemDomId ); + setActiveCompositeId( targetCompositeActiveId ); + ( + document.querySelector( + `#${ targetCompositeActiveId }` + ) as HTMLElement | undefined + )?.focus(); + } - const selectPreviousItemDropdownTrigger = () => { - selectItemDropdownTrigger( - Math.min( data.length - 1, Math.max( 0, activeItemIndex - 1 ) ) - ); - }; - const selectNextItemDropdownTrigger = () => { - selectItemDropdownTrigger( - Math.min( data.length - 1, Math.max( 0, activeItemIndex + 1 ) ) - ); - }; + if ( event.key === 'ArrowDown' ) { + event.preventDefault(); + selectItemDropdownTrigger( + Math.min( + data.length - 1, + Math.max( 0, activeItemIndex + 1 ) + ) + ); + } + if ( event.key === 'ArrowUp' ) { + event.preventDefault(); + selectItemDropdownTrigger( + Math.min( + data.length - 1, + Math.max( 0, activeItemIndex - 1 ) + ) + ); + } + }, + [ activeItemIndex, data, getItemDomId ] + ); const hasData = data?.length; if ( ! hasData ) { @@ -468,12 +466,7 @@ export default function ViewList< Item >( props: ViewListProps< Item > ) { mediaField={ mediaField } primaryField={ primaryField } visibleFields={ visibleFields } - selectPreviousItemDropdownTrigger={ - selectPreviousItemDropdownTrigger - } - selectNextItemDropdownTrigger={ - selectNextItemDropdownTrigger - } + onDropdownTriggerKeyDown={ onDropdownTriggerKeyDown } /> ); } ) } From 5e6dd9615d1d2b74e044b5e416b8021422cbb544 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Tue, 3 Sep 2024 09:37:45 +0200 Subject: [PATCH 04/10] Simplify getItemDomId --- .../dataviews/src/dataviews-layouts/list/index.tsx | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/packages/dataviews/src/dataviews-layouts/list/index.tsx b/packages/dataviews/src/dataviews-layouts/list/index.tsx index eecff5a7604714..31756259b5e13c 100644 --- a/packages/dataviews/src/dataviews-layouts/list/index.tsx +++ b/packages/dataviews/src/dataviews-layouts/list/index.tsx @@ -311,13 +311,7 @@ export default function ViewList< Item >( props: ViewListProps< Item > ) { const baseId = useInstanceId( ViewList, 'view-list' ); const getItemDomId = useCallback( - ( item?: Item ) => { - if ( ! item ) { - return; - } - - return `${ baseId }-${ getItemId( item ) }`; - }, + ( item: Item ) => `${ baseId }-${ getItemId( item ) }`, [ baseId, getItemId ] ); @@ -332,7 +326,7 @@ export default function ViewList< Item >( props: ViewListProps< Item > ) { string | null | undefined >( // By default, the active composite item is the selected one. - getItemDomId( selectedItem ) + selectedItem ? getItemDomId( selectedItem ) : undefined ); const mediaField = fields.find( @@ -452,9 +446,7 @@ export default function ViewList< Item >( props: ViewListProps< Item > ) { setActiveId={ setActiveCompositeId } > { data.map( ( item ) => { - // Since `item` is guaranteed not to be undefined, we can safely - // let typescript know that `id` is not undefined either. - const id = getItemDomId( item )!; + const id = getItemDomId( item ); return ( Date: Tue, 3 Sep 2024 09:41:21 +0200 Subject: [PATCH 05/10] Further simplify dropdown menu trigger keydown handler --- .../src/dataviews-layouts/list/index.tsx | 76 +++++++++---------- 1 file changed, 34 insertions(+), 42 deletions(-) diff --git a/packages/dataviews/src/dataviews-layouts/list/index.tsx b/packages/dataviews/src/dataviews-layouts/list/index.tsx index 31756259b5e13c..2001b6e0f61c1c 100644 --- a/packages/dataviews/src/dataviews-layouts/list/index.tsx +++ b/packages/dataviews/src/dataviews-layouts/list/index.tsx @@ -354,70 +354,62 @@ export default function ViewList< Item >( props: ViewListProps< Item > ) { const previousActiveItemIndex = usePrevious( activeItemIndex ); const isActiveIdInList = activeItemIndex !== -1; + const selectCompositeItem = useCallback( + ( + targetIndex: number, + // Allows invokers to specify a custom function to generate the + // target composite item ID (e.g. for the dropdown menu trigger). + getCompositeId = ( id: string ) => id + ) => { + // Clamping between 0 and data.length - 1 to avoid out of bounds. + const clampedIndex = Math.min( + data.length - 1, + Math.max( 0, targetIndex ) + ); + const domId = getItemDomId( data[ clampedIndex ] ); + const targetCompositeItemId = getCompositeId( domId ); + + setActiveCompositeId( targetCompositeItemId ); + document.getElementById( targetCompositeItemId )?.focus(); + }, + [ data, getItemDomId ] + ); + + // Select a new active composite item when the current active item + // is removed from the list. useEffect( () => { if ( ! isActiveIdInList ) { // By picking `previousActiveItemIndex` as the next item index, we are // basically picking the item that would have been after the deleted one. // If the previously active (and removed) item was the last of the list, // we will select the item before it — which is the new last item. - // Clamping between 0 and data.length - 1 to avoid out of bounds. - const clampedPreviousActiveItemIndex = Math.max( - 0, - Math.min( previousActiveItemIndex ?? 0, data.length - 1 ) - ); - const nextActiveDataItem = data[ clampedPreviousActiveItemIndex ]; - - const nextCompositeActiveId = getItemDomId( nextActiveDataItem ); - setActiveCompositeId( nextCompositeActiveId ); - ( - document.querySelector( `#${ nextCompositeActiveId }` ) as - | HTMLElement - | undefined - )?.focus(); + selectCompositeItem( previousActiveItemIndex ?? 0 ); } - }, [ data, previousActiveItemIndex, getItemDomId, isActiveIdInList ] ); + }, [ previousActiveItemIndex, isActiveIdInList, selectCompositeItem ] ); // Prevent the default behavior (open dropdown menu) and instead select the // dropdown menu trigger on the previous/next row. // https://github.com/ariakit/ariakit/issues/3768 const onDropdownTriggerKeyDown = useCallback( ( event: React.KeyboardEvent< HTMLButtonElement > ) => { - function selectItemDropdownTrigger( itemIndex: number ) { - const item = data[ itemIndex ]; - const itemDomId = getItemDomId( item ); - if ( ! itemDomId ) { - return; - } - const targetCompositeActiveId = - generateDropdownTriggerCompositeId( itemDomId ); - setActiveCompositeId( targetCompositeActiveId ); - ( - document.querySelector( - `#${ targetCompositeActiveId }` - ) as HTMLElement | undefined - )?.focus(); - } - if ( event.key === 'ArrowDown' ) { + // Select the dropdown menu trigger item in the next row. event.preventDefault(); - selectItemDropdownTrigger( - Math.min( - data.length - 1, - Math.max( 0, activeItemIndex + 1 ) - ) + selectCompositeItem( + activeItemIndex + 1, + generateDropdownTriggerCompositeId ); } if ( event.key === 'ArrowUp' ) { + // Select the dropdown menu trigger item in the previous row. event.preventDefault(); - selectItemDropdownTrigger( - Math.min( - data.length - 1, - Math.max( 0, activeItemIndex - 1 ) - ) + selectCompositeItem( + activeItemIndex - 1, + generateDropdownTriggerCompositeId ); } }, - [ activeItemIndex, data, getItemDomId ] + [ selectCompositeItem, activeItemIndex ] ); const hasData = data?.length; From 2a23d34c484429063881ec3fd1ba9b5f075b71cb Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Tue, 3 Sep 2024 10:22:54 +0200 Subject: [PATCH 06/10] Extract primary action grid cell to separate component --- .../src/dataviews-layouts/list/index.tsx | 129 +++++++++--------- 1 file changed, 68 insertions(+), 61 deletions(-) diff --git a/packages/dataviews/src/dataviews-layouts/list/index.tsx b/packages/dataviews/src/dataviews-layouts/list/index.tsx index 2001b6e0f61c1c..10f14903dcea40 100644 --- a/packages/dataviews/src/dataviews-layouts/list/index.tsx +++ b/packages/dataviews/src/dataviews-layouts/list/index.tsx @@ -59,6 +59,68 @@ function generateDropdownTriggerCompositeId( domId: string ) { return `${ domId }-dropdown`; } +function PrimaryActionGridCell< Item >( { + primaryAction, + id, + item, +}: { + id: string; + primaryAction: Action< Item >; + item: Item; +} ) { + const registry = useRegistry(); + const [ isModalOpen, setIsModalOpen ] = useState( false ); + + const compositeItemId = `${ id }-${ primaryAction.id }`; + + const label = + typeof primaryAction.label === 'string' + ? primaryAction.label + : primaryAction.label( [ item ] ); + + return 'RenderModal' in primaryAction ? ( +
    + setIsModalOpen( true ) } + /> + } + > + { isModalOpen && ( + + action={ primaryAction } + items={ [ item ] } + closeModal={ () => setIsModalOpen( false ) } + /> + ) } + +
    + ) : ( +
    + { + primaryAction.callback( [ item ], { registry } ); + } } + /> + } + /> +
    + ); +} + function ListItem< Item >( { actions, id, @@ -70,7 +132,6 @@ function ListItem< Item >( { visibleFields, onDropdownTriggerKeyDown, }: ListViewItemProps< Item > ) { - const registry = useRegistry(); const itemRef = useRef< HTMLElement >( null ); const labelId = `${ id }-label`; const descriptionId = `${ id }-description`; @@ -108,13 +169,6 @@ function ListItem< Item >( { }; }, [ actions, item ] ); - const [ isModalOpen, setIsModalOpen ] = useState( false ); - const primaryActionLabel = - primaryAction && - ( typeof primaryAction.label === 'string' - ? primaryAction.label - : primaryAction.label( [ item ] ) ); - const renderedMediaField = mediaField?.render ? ( ) : ( @@ -206,60 +260,13 @@ function ListItem< Item >( { width: 'auto', } } > - { primaryAction && 'RenderModal' in primaryAction && ( -
    - - setIsModalOpen( true ) - } - /> - } - > - { isModalOpen && ( - - action={ primaryAction } - items={ [ item ] } - closeModal={ () => - setIsModalOpen( false ) - } - /> - ) } - -
    + { primaryAction && ( + ) } - { primaryAction && - ! ( 'RenderModal' in primaryAction ) && ( -
    - { - primaryAction.callback( - [ item ], - { registry } - ); - } } - /> - } - /> -
    - ) }
    Date: Tue, 3 Sep 2024 10:32:32 +0200 Subject: [PATCH 07/10] Sort render function order for better logic flow --- .../src/dataviews-layouts/list/index.tsx | 31 +++++++++---------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/packages/dataviews/src/dataviews-layouts/list/index.tsx b/packages/dataviews/src/dataviews-layouts/list/index.tsx index 10f14903dcea40..fc817a237c9e19 100644 --- a/packages/dataviews/src/dataviews-layouts/list/index.tsx +++ b/packages/dataviews/src/dataviews-layouts/list/index.tsx @@ -314,28 +314,12 @@ export default function ViewList< Item >( props: ViewListProps< Item > ) { selection, view, } = props; - const baseId = useInstanceId( ViewList, 'view-list' ); - const getItemDomId = useCallback( - ( item: Item ) => `${ baseId }-${ getItemId( item ) }`, - [ baseId, getItemId ] - ); - const selectedItem = data?.findLast( ( item ) => selection.includes( getItemId( item ) ) ); - const onSelect = ( item: Item ) => - onChangeSelection( [ getItemId( item ) ] ); - - const [ activeCompositeId, setActiveCompositeId ] = useState< - string | null | undefined - >( - // By default, the active composite item is the selected one. - selectedItem ? getItemDomId( selectedItem ) : undefined - ); - const mediaField = fields.find( ( field ) => field.id === view.layout?.mediaField ); @@ -351,6 +335,21 @@ export default function ViewList< Item >( props: ViewListProps< Item > ) { ) ); + const onSelect = ( item: Item ) => + onChangeSelection( [ getItemId( item ) ] ); + + const getItemDomId = useCallback( + ( item: Item ) => `${ baseId }-${ getItemId( item ) }`, + [ baseId, getItemId ] + ); + + const [ activeCompositeId, setActiveCompositeId ] = useState< + string | null | undefined + >( + // By default, the active composite item is the selected one. + selectedItem ? getItemDomId( selectedItem ) : undefined + ); + const activeItemIndex = data.findIndex( ( item ) => { const itemCompositeIdPrefix = getItemDomId( item ); return ( From b3472e566b63833d55f0239d759dac8b669e60cc Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Tue, 3 Sep 2024 10:51:06 +0200 Subject: [PATCH 08/10] Extract more logic to generate IDs + always use ID prefix --- .../src/dataviews-layouts/list/index.tsx | 82 ++++++++++++------- 1 file changed, 53 insertions(+), 29 deletions(-) diff --git a/packages/dataviews/src/dataviews-layouts/list/index.tsx b/packages/dataviews/src/dataviews-layouts/list/index.tsx index fc817a237c9e19..4c5b82cb2bbadc 100644 --- a/packages/dataviews/src/dataviews-layouts/list/index.tsx +++ b/packages/dataviews/src/dataviews-layouts/list/index.tsx @@ -38,7 +38,7 @@ import type { Action, NormalizedField, ViewListProps } from '../../types'; interface ListViewItemProps< Item > { actions: Action< Item >[]; - id: string; + idPrefix: string; isSelected: boolean; item: Item; mediaField?: NormalizedField< Item >; @@ -55,23 +55,35 @@ const { DropdownMenuV2: DropdownMenu, } = unlock( componentsPrivateApis ); -function generateDropdownTriggerCompositeId( domId: string ) { - return `${ domId }-dropdown`; +function generateItemWrapperCompositeId( idPrefix: string ) { + return `${ idPrefix }-item-wrapper`; +} +function generatePrimaryActionCompositeId( + idPrefix: string, + primaryActionId: string +) { + return `${ idPrefix }-primary-action-${ primaryActionId }`; +} +function generateDropdownTriggerCompositeId( idPrefix: string ) { + return `${ idPrefix }-dropdown`; } function PrimaryActionGridCell< Item >( { + idPrefix, primaryAction, - id, item, }: { - id: string; + idPrefix: string; primaryAction: Action< Item >; item: Item; } ) { const registry = useRegistry(); const [ isModalOpen, setIsModalOpen ] = useState( false ); - const compositeItemId = `${ id }-${ primaryAction.id }`; + const compositeItemId = generatePrimaryActionCompositeId( + idPrefix, + primaryAction.id + ); const label = typeof primaryAction.label === 'string' @@ -123,7 +135,7 @@ function PrimaryActionGridCell< Item >( { function ListItem< Item >( { actions, - id, + idPrefix, isSelected, item, mediaField, @@ -133,8 +145,8 @@ function ListItem< Item >( { onDropdownTriggerKeyDown, }: ListViewItemProps< Item > ) { const itemRef = useRef< HTMLElement >( null ); - const labelId = `${ id }-label`; - const descriptionId = `${ id }-description`; + const labelId = `${ idPrefix }-label`; + const descriptionId = `${ idPrefix }-description`; const [ isHovered, setIsHovered ] = useState( false ); const handleMouseEnter = () => { @@ -200,7 +212,7 @@ function ListItem< Item >( { } role="button" - id={ id } + id={ generateItemWrapperCompositeId( idPrefix ) } aria-pressed={ isSelected } aria-labelledby={ labelId } aria-describedby={ descriptionId } @@ -262,7 +274,7 @@ function ListItem< Item >( { > { primaryAction && ( @@ -272,7 +284,7 @@ function ListItem< Item >( { trigger={ ( props: ViewListProps< Item > ) { const onSelect = ( item: Item ) => onChangeSelection( [ getItemId( item ) ] ); - const getItemDomId = useCallback( + const generateCompositeItemIdPrefix = useCallback( ( item: Item ) => `${ baseId }-${ getItemId( item ) }`, [ baseId, getItemId ] ); + const isActiveCompositeItem = useCallback( + ( item: Item, idToCheck: string ) => { + // All composite items use the same prefix in their IDs. + return idToCheck.startsWith( + generateCompositeItemIdPrefix( item ) + ); + }, + [ generateCompositeItemIdPrefix ] + ); + + // Controlled state for the active composite item. const [ activeCompositeId, setActiveCompositeId ] = useState< string | null | undefined >( // By default, the active composite item is the selected one. - selectedItem ? getItemDomId( selectedItem ) : undefined + selectedItem ? generateCompositeItemIdPrefix( selectedItem ) : undefined ); - const activeItemIndex = data.findIndex( ( item ) => { - const itemCompositeIdPrefix = getItemDomId( item ); - return ( - !! itemCompositeIdPrefix && - activeCompositeId?.startsWith( itemCompositeIdPrefix ) - ); - } ); + const activeItemIndex = data.findIndex( ( item ) => + isActiveCompositeItem( item, activeCompositeId ?? '' ) + ); const previousActiveItemIndex = usePrevious( activeItemIndex ); const isActiveIdInList = activeItemIndex !== -1; @@ -364,21 +383,23 @@ export default function ViewList< Item >( props: ViewListProps< Item > ) { ( targetIndex: number, // Allows invokers to specify a custom function to generate the - // target composite item ID (e.g. for the dropdown menu trigger). - getCompositeId = ( id: string ) => id + // target composite item ID + generateCompositeId: ( idPrefix: string ) => string ) => { // Clamping between 0 and data.length - 1 to avoid out of bounds. const clampedIndex = Math.min( data.length - 1, Math.max( 0, targetIndex ) ); - const domId = getItemDomId( data[ clampedIndex ] ); - const targetCompositeItemId = getCompositeId( domId ); + const itemIdPrefix = generateCompositeItemIdPrefix( + data[ clampedIndex ] + ); + const targetCompositeItemId = generateCompositeId( itemIdPrefix ); setActiveCompositeId( targetCompositeItemId ); document.getElementById( targetCompositeItemId )?.focus(); }, - [ data, getItemDomId ] + [ data, generateCompositeItemIdPrefix ] ); // Select a new active composite item when the current active item @@ -389,7 +410,10 @@ export default function ViewList< Item >( props: ViewListProps< Item > ) { // basically picking the item that would have been after the deleted one. // If the previously active (and removed) item was the last of the list, // we will select the item before it — which is the new last item. - selectCompositeItem( previousActiveItemIndex ?? 0 ); + selectCompositeItem( + previousActiveItemIndex ?? 0, + generateItemWrapperCompositeId + ); } }, [ previousActiveItemIndex, isActiveIdInList, selectCompositeItem ] ); @@ -444,11 +468,11 @@ export default function ViewList< Item >( props: ViewListProps< Item > ) { setActiveId={ setActiveCompositeId } > { data.map( ( item ) => { - const id = getItemDomId( item ); + const id = generateCompositeItemIdPrefix( item ); return ( Date: Tue, 3 Sep 2024 11:37:53 +0200 Subject: [PATCH 09/10] Fix buggy initial active composite ID logic --- .../src/dataviews-layouts/list/index.tsx | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/packages/dataviews/src/dataviews-layouts/list/index.tsx b/packages/dataviews/src/dataviews-layouts/list/index.tsx index 4c5b82cb2bbadc..93a1d3e944f307 100644 --- a/packages/dataviews/src/dataviews-layouts/list/index.tsx +++ b/packages/dataviews/src/dataviews-layouts/list/index.tsx @@ -368,10 +368,18 @@ export default function ViewList< Item >( props: ViewListProps< Item > ) { // Controlled state for the active composite item. const [ activeCompositeId, setActiveCompositeId ] = useState< string | null | undefined - >( - // By default, the active composite item is the selected one. - selectedItem ? generateCompositeItemIdPrefix( selectedItem ) : undefined - ); + >( undefined ); + + // Update the active composite item when the selected item changes. + useEffect( () => { + if ( selectedItem ) { + setActiveCompositeId( + generateItemWrapperCompositeId( + generateCompositeItemIdPrefix( selectedItem ) + ) + ); + } + }, [ selectedItem, generateCompositeItemIdPrefix ] ); const activeItemIndex = data.findIndex( ( item ) => isActiveCompositeItem( item, activeCompositeId ?? '' ) @@ -405,17 +413,20 @@ export default function ViewList< Item >( props: ViewListProps< Item > ) { // Select a new active composite item when the current active item // is removed from the list. useEffect( () => { - if ( ! isActiveIdInList ) { + const wasActiveIdInList = + previousActiveItemIndex !== undefined && + previousActiveItemIndex !== -1; + if ( ! isActiveIdInList && wasActiveIdInList ) { // By picking `previousActiveItemIndex` as the next item index, we are // basically picking the item that would have been after the deleted one. // If the previously active (and removed) item was the last of the list, // we will select the item before it — which is the new last item. selectCompositeItem( - previousActiveItemIndex ?? 0, + previousActiveItemIndex, generateItemWrapperCompositeId ); } - }, [ previousActiveItemIndex, isActiveIdInList, selectCompositeItem ] ); + }, [ isActiveIdInList, selectCompositeItem, previousActiveItemIndex ] ); // Prevent the default behavior (open dropdown menu) and instead select the // dropdown menu trigger on the previous/next row. From 0033f7a4f36e00812b066bed97628c199effee27 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Tue, 3 Sep 2024 17:06:05 +0200 Subject: [PATCH 10/10] Add key to primary action gridcell --- packages/dataviews/src/dataviews-layouts/list/index.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dataviews/src/dataviews-layouts/list/index.tsx b/packages/dataviews/src/dataviews-layouts/list/index.tsx index 93a1d3e944f307..00146c3ee35220 100644 --- a/packages/dataviews/src/dataviews-layouts/list/index.tsx +++ b/packages/dataviews/src/dataviews-layouts/list/index.tsx @@ -91,7 +91,7 @@ function PrimaryActionGridCell< Item >( { : primaryAction.label( [ item ] ); return 'RenderModal' in primaryAction ? ( -
    +