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

[Data Views] Updating keyboard navigation in list layouts #59637

Merged
merged 1 commit into from Mar 21, 2024
Merged

[Data Views] Updating keyboard navigation in list layouts #59637

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 18 additions & 7 deletions packages/dataviews/src/style.scss
View file
Open in desktop
Expand Up @@ -340,6 +340,7 @@

li {
margin: 0;
cursor: pointer;

.dataviews-view-list__item-wrapper {
position: relative;
Expand All @@ -355,14 +356,21 @@
background: $gray-100;
height: 1px;
}
}

&:not(.is-selected):hover {
color: var(--wp-admin-theme-color);
> * {
width: 100%;
}
}

.dataviews-view-list__primary-field,
.dataviews-view-list__fields {
&:not(.is-selected) {
&:hover,
&:focus-within {
color: var(--wp-admin-theme-color);

.dataviews-view-list__primary-field,
.dataviews-view-list__fields {
color: var(--wp-admin-theme-color);
}
}
}
}
Expand All @@ -388,7 +396,8 @@
.dataviews-view-list__item {
padding: $grid-unit-15 0 $grid-unit-15 $grid-unit-30;
width: 100%;
cursor: pointer;
scroll-margin: $grid-unit-10 0;

&:focus {
&::before {
position: absolute;
Expand Down Expand Up @@ -449,7 +458,9 @@
line-height: $grid-unit-20;

.dataviews-view-list__field {
&:empty {
margin: 0;

&:has(.dataviews-view-list__field-value:empty) {
display: none;
}
}
Expand Down
228 changes: 159 additions & 69 deletions packages/dataviews/src/view-list.js
View file
Open in desktop
Expand Up @@ -6,17 +6,135 @@ import classNames from 'classnames';
/**
* WordPress dependencies
*/
import { useAsyncList } from '@wordpress/compose';
import { useAsyncList, useInstanceId } from '@wordpress/compose';
import {
__experimentalHStack as HStack,
__experimentalVStack as VStack,
privateApis as componentsPrivateApis,
Button,
Spinner,
VisuallyHidden,
} from '@wordpress/components';
import { ENTER, SPACE } from '@wordpress/keycodes';
import { useCallback, useEffect, useRef } from '@wordpress/element';
import { info } from '@wordpress/icons';
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import { unlock } from './lock-unlock';

const {
useCompositeStoreV2: useCompositeStore,
CompositeV2: Composite,
CompositeItemV2: CompositeItem,
CompositeRowV2: CompositeRow,
} = unlock( componentsPrivateApis );

function ListItem( {
id,
item,
isSelected,
onSelect,
onDetailsChange,
mediaField,
primaryField,
visibleFields,
} ) {
const itemRef = useRef( null );
const labelId = `${ id }-label`;
const descriptionId = `${ id }-description`;

useEffect( () => {
if ( isSelected ) {
itemRef.current?.scrollIntoView( {
behavior: 'auto',
block: 'nearest',
inline: 'nearest',
} );
}
}, [ isSelected ] );

return (
<CompositeRow
ref={ itemRef }
render={ <li /> }
role="row"
className={ classNames( {
'is-selected': isSelected,
} ) }
>
<HStack className="dataviews-view-list__item-wrapper">
<div role="gridcell">
<CompositeItem
render={ <div /> }
role="button"
id={ id }
aria-pressed={ isSelected }
aria-labelledby={ labelId }
aria-describedby={ descriptionId }
className="dataviews-view-list__item"
onClick={ () => onSelect( item ) }
>
<HStack
spacing={ 3 }
justify="start"
alignment="flex-start"
>
<div className="dataviews-view-list__media-wrapper">
{ mediaField?.render( { item } ) || (
<div className="dataviews-view-list__media-placeholder"></div>
) }
</div>
<VStack spacing={ 1 }>
<span
className="dataviews-view-list__primary-field"
id={ labelId }
>
{ primaryField?.render( { item } ) }
</span>
<div
className="dataviews-view-list__fields"
id={ descriptionId }
>
{ visibleFields.map( ( field ) => (
<p
key={ field.id }
className="dataviews-view-list__field"
>
<VisuallyHidden
as="span"
className="dataviews-view-list__field-label"
>
{ field.header }
</VisuallyHidden>
<span className="dataviews-view-list__field-value">
{ field.render( { item } ) }
</span>
</p>
) ) }
</div>
</VStack>
</HStack>
</CompositeItem>
</div>
{ onDetailsChange && (
<div role="gridcell">
<CompositeItem
render={ <Button /> }
className="dataviews-view-list__details-button"
onClick={ () => onDetailsChange( [ item ] ) }
icon={ info }
label={ __( 'View details' ) }
size="compact"
/>
</div>
) }
</HStack>
</CompositeRow>
);
}

export default function ViewList( {
view,
fields,
Expand All @@ -27,9 +145,15 @@ export default function ViewList( {
onDetailsChange,
selection,
deferredRendering,
id: preferredId,
} ) {
const baseId = useInstanceId( ViewList, 'view-list', preferredId );
const shownData = useAsyncList( data, { step: 3 } );
const usedData = deferredRendering ? shownData : data;
const selectedItem = usedData?.findLast( ( item ) =>
Copy link
Member

Choose a reason for hiding this comment

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

This changes how selection works for the list item. Particularly, in the case of some items being pre-selected from a different view and then switch to the list view.

Before:

Gravacao.do.ecra.2024-03-08.as.14.31.42.mov

After:

Gravacao.do.ecra.2024-03-08.as.14.30.12.mov

Until we figure out whether it makes sense to have multiple selected elements in the list view, this is a good approach.

selection.includes( item.id )
);

const mediaField = fields.find(
( field ) => field.id === view.layout.mediaField
);
Expand All @@ -44,12 +168,19 @@ export default function ViewList( {
)
);

const onEnter = ( item ) => ( event ) => {
const { keyCode } = event;
if ( [ ENTER, SPACE ].includes( keyCode ) ) {
onSelectionChange( [ item ] );
}
};
const onSelect = useCallback(
( item ) => onSelectionChange( [ item ] ),
[ onSelectionChange ]
);

const getItemDomId = useCallback(
( item ) => ( item ? `${ baseId }-${ getItemId( item ) }` : undefined ),
[ baseId, getItemId ]
);

const store = useCompositeStore( {
defaultActiveId: getItemDomId( selectedItem ),
} );

const hasData = usedData?.length;
if ( ! hasData ) {
Expand All @@ -68,70 +199,29 @@ export default function ViewList( {
}

return (
<ul className="dataviews-view-list">
<Composite
id={ baseId }
render={ <ul /> }
className="dataviews-view-list"
role="grid"
store={ store }
>
{ usedData.map( ( item ) => {
const id = getItemDomId( item );
Copy link
Member

Choose a reason for hiding this comment

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

Why not using getItemId directly? By definition, that's each item's unique identifier.

Copy link
Member

@oandregal oandregal Mar 8, 2024 •
edited

Choose a reason for hiding this comment

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

Is this something related to how Composite/Ariakit works? Otherwise, I don't follow what the ids are for at this level. Judging by what I see at runtime, I presume 1) Ariakit needs this tracking and 2) we want to make sure the id doesn't conflict with any other. If so, can we go simpler and do something along the lines of list-item-${getItemId( item )}?

Copy link
Member

Choose a reason for hiding this comment

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

I'm largely unfamiliar with Ariakit's Composite, but I find it weird that it 1) uses DOM ids to track the items and 2) the concept of a store attached to a component.

I understand that, by using Ariakit's Composite, we want to hide a certain layer of complexity (handle onClick/onKeydown events + tab/arrow-key mechanics) though I'd argue that complexity is familiar for most people while the complexity added by Ariakit's Composite mechanics is unfamiliar for most. In terms of maintenance and evolution of this code, I feel a bit uneasy about this trade-off.

Copy link
Member

Choose a reason for hiding this comment

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

Asked around to some folks and others think this is fine.

return (
<li
key={ getItemId( item ) }
className={ classNames( {
'is-selected': selection.includes( item.id ),
} ) }
>
<HStack className="dataviews-view-list__item-wrapper">
<div
role="button"
tabIndex={ 0 }
aria-pressed={ selection.includes( item.id ) }
onKeyDown={ onEnter( item ) }
className="dataviews-view-list__item"
onClick={ () => onSelectionChange( [ item ] ) }
>
<HStack
spacing={ 3 }
justify="start"
alignment="flex-start"
>
<div className="dataviews-view-list__media-wrapper">
{ mediaField?.render( { item } ) || (
<div className="dataviews-view-list__media-placeholder"></div>
) }
</div>
<VStack spacing={ 1 }>
<span className="dataviews-view-list__primary-field">
{ primaryField?.render( { item } ) }
</span>
<div className="dataviews-view-list__fields">
{ visibleFields.map( ( field ) => {
return (
<span
key={ field.id }
className="dataviews-view-list__field"
>
{ field.render( {
item,
} ) }
</span>
);
} ) }
</div>
</VStack>
</HStack>
</div>
{ onDetailsChange && (
<Button
className="dataviews-view-list__details-button"
onClick={ () =>
onDetailsChange( [ item ] )
}
icon={ info }
label={ __( 'View details' ) }
size="compact"
/>
) }
</HStack>
</li>
<ListItem
key={ id }
id={ id }
item={ item }
isSelected={ item === selectedItem }
onSelect={ onSelect }
onDetailsChange={ onDetailsChange }
mediaField={ mediaField }
primaryField={ primaryField }
visibleFields={ visibleFields }
/>
);
} ) }
</ul>
</Composite>
);
}