Skip to content
Merged
1 change: 1 addition & 0 deletions packages/dataviews/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ Properties:
- `showMedia`: Whether the media should be shown in the UI. `true` by default.
- `showDescription`: Whether the description should be shown in the UI. `true` by default.
- `showLevels`: Whether to display the hierarchical levels for the data. `false` by default. See related `getItemLevel` DataView prop.
- `groupByField`: The id of the field used for grouping the dataset. So far, only the `grid` layout supports grouping.
- `fields`: a list of remaining field `id` that are visible in the UI and the specific order in which they are displayed.
- `layout`: config that is specific to a particular layout type.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -326,3 +326,40 @@ export const CustomPerPageSizes = () => {
/>
);
};

export const GroupedGridLayout = () => {
const [ view, setView ] = useState< View >( {
type: LAYOUT_GRID,
search: '',
page: 1,
perPage: 20,
filters: [],
fields: [ 'satellites' ],
titleField: 'title',
descriptionField: 'description',
mediaField: 'image',
groupByField: 'type',
layout: {
badgeFields: [ 'satellites' ],
},
} );
const { data: shownData, paginationInfo } = useMemo( () => {
return filterSortAndPaginate( data, view, fields );
}, [ view ] );
Comment on lines +346 to +348
Copy link
Member

Choose a reason for hiding this comment

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

What if just the data changes? Don't we risk showing obsolete memoized data that way?

I know this is in a story, but folks tend to copy code from stories, so we need to be mindful about it.

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case, data and fields won't change because they are static arrays.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure I was clear enough - my worry is about people copying the code from stories and it potentially leading to the issues I mentioned.

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand that viewpoint, but I'm reluctant to add unnecessary code to cover misuse — that's not something I've seen us doing.

return (
<DataViews
getItemId={ ( item ) => item.id.toString() }
paginationInfo={ paginationInfo }
data={ shownData }
view={ view }
fields={ fields }
onChangeView={ setView }
actions={ actions }
defaultLayouts={ {
[ LAYOUT_GRID ]: {},
[ LAYOUT_LIST ]: {},
[ LAYOUT_TABLE ]: {},
} }
/>
);
};
179 changes: 135 additions & 44 deletions packages/dataviews/src/dataviews-layouts/grid/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
FlexItem,
privateApis as componentsPrivateApis,
} from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import { __, sprintf } from '@wordpress/i18n';
import { useInstanceId } from '@wordpress/compose';

/**
Expand Down Expand Up @@ -293,51 +293,142 @@ function ViewGrid< Item >( {
gridTemplateColumns: `repeat(${ usedPreviewSize }, minmax(0, 1fr))`,
}
: {};

const groupField = view.groupByField
? fields.find( ( f ) => f.id === view.groupByField )
: null;

// Group data by groupByField if specified
const dataByGroup = groupField
? data.reduce( ( groups: { [ key: string ]: typeof data }, item ) => {
const groupName = groupField.getValue( { item } );
if ( ! groups[ groupName ] ) {
groups[ groupName ] = [];
Copy link
Member

Choose a reason for hiding this comment

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

Theoretically, keys could conflict with the existing properties of object. Isn't it safer to use a Map (and potentially more semantic) rather than a plain object?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to use a Map at 8ac0e2c

}
groups[ groupName ].push( item );
return groups;
}, {} )
: null;

return (
<>
{ hasData && (
<Grid
gap={ 8 }
columns={ 2 }
alignment="top"
className={ clsx( 'dataviews-view-grid', className ) }
style={ gridStyle }
aria-busy={ isLoading }
>
{ data.map( ( item ) => {
return (
<GridItem
key={ getItemId( item ) }
view={ view }
selection={ selection }
onChangeSelection={ onChangeSelection }
onClickItem={ onClickItem }
isItemClickable={ isItemClickable }
renderItemLink={ renderItemLink }
getItemId={ getItemId }
item={ item }
actions={ actions }
mediaField={ mediaField }
titleField={ titleField }
descriptionField={ descriptionField }
regularFields={ regularFields }
badgeFields={ badgeFields }
hasBulkActions={ hasBulkActions }
/>
);
} ) }
</Grid>
) }
{ ! hasData && (
<div
className={ clsx( {
'dataviews-loading': isLoading,
'dataviews-no-results': ! isLoading,
} ) }
>
<p>{ isLoading ? <Spinner /> : __( 'No results' ) }</p>
</div>
) }
{
// Render multiple groups.
hasData && groupField && dataByGroup && (
<VStack spacing={ 4 }>
{ Object.entries( dataByGroup ).map(
( [ groupName, groupItems ] ) => (
<VStack key={ groupName } spacing={ 2 }>
<h3 className="dataviews-view-grid__group-header">
{ sprintf(
// translators: 1: The label of the field e.g. "Date". 2: The value of the field, e.g.: "May 2022".
__( '%1$s: %2$s' ),
groupField.label,
groupName
) }
</h3>
<Grid
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like this just duplicates what we have in the bottom of the component, only diff seems to be the data used no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it is. I didn't think this would benefit from a further abstraction.

Copy link
Contributor

Choose a reason for hiding this comment

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

No need for something big really here to be honest, just a function or subcomponent.

gap={ 8 }
columns={ 2 }
alignment="top"
className={ clsx(
'dataviews-view-grid',
className
) }
style={ gridStyle }
aria-busy={ isLoading }
>
{ groupItems.map( ( item ) => {
return (
<GridItem
key={ getItemId( item ) }
view={ view }
selection={ selection }
onChangeSelection={
onChangeSelection
}
onClickItem={ onClickItem }
isItemClickable={
isItemClickable
}
renderItemLink={
renderItemLink
}
getItemId={ getItemId }
item={ item }
actions={ actions }
mediaField={ mediaField }
titleField={ titleField }
descriptionField={
descriptionField
}
regularFields={
regularFields
}
badgeFields={ badgeFields }
hasBulkActions={
hasBulkActions
}
/>
);
} ) }
</Grid>
</VStack>
)
) }
</VStack>
)
}

{
// Render a single grid with all data.
hasData && ! dataByGroup && (
<Grid
gap={ 8 }
columns={ 2 }
alignment="top"
className={ clsx( 'dataviews-view-grid', className ) }
style={ gridStyle }
aria-busy={ isLoading }
>
{ data.map( ( item ) => {
return (
<GridItem
key={ getItemId( item ) }
view={ view }
selection={ selection }
onChangeSelection={ onChangeSelection }
onClickItem={ onClickItem }
isItemClickable={ isItemClickable }
renderItemLink={ renderItemLink }
getItemId={ getItemId }
item={ item }
actions={ actions }
mediaField={ mediaField }
titleField={ titleField }
descriptionField={ descriptionField }
regularFields={ regularFields }
badgeFields={ badgeFields }
hasBulkActions={ hasBulkActions }
/>
);
} ) }
</Grid>
)
}
{
// Render empty state.
! hasData && (
<div
className={ clsx( {
'dataviews-loading': isLoading,
'dataviews-no-results': ! isLoading,
} ) }
>
<p>{ isLoading ? <Spinner /> : __( 'No results' ) }</p>
</div>
)
}
</>
);
}
Expand Down
8 changes: 8 additions & 0 deletions packages/dataviews/src/dataviews-layouts/grid/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -166,3 +166,11 @@
.dataviews-view-grid__media--clickable {
cursor: pointer;
}

.dataviews-view-grid__group-header {
font-size: 16px;
font-weight: 600;
color: $gray-900;
margin: 0 0 $grid-unit-10 0;
padding: 0 $grid-unit-60;
}
34 changes: 25 additions & 9 deletions packages/dataviews/src/filter-and-sort-data-view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -375,16 +375,32 @@ export function filterSortAndPaginate< Item >(
}

// Handle sorting.
if ( view.sort ) {
const fieldId = view.sort.field;
const fieldToSort = _fields.find( ( field ) => {
return field.id === fieldId;
} );
if ( fieldToSort ) {
filteredData.sort( ( a, b ) => {
return fieldToSort.sort( a, b, view.sort?.direction ?? 'desc' );
if ( view.sort || view.groupByField ) {
filteredData.sort( ( a, b ) => {
// Sort by the group by field.
const groupByField = _fields.find( ( field ) => {
return field.id === view.groupByField;
} );
}
if ( view.groupByField && groupByField ) {
const groupCompare = groupByField.sort( a, b, 'asc' );

// If items are in different groups, return the group comparison result.
// Otherwise, fall back to sorting by the sort field.
if ( groupCompare !== 0 ) {
return groupCompare;
}
}

// Sort by the sort field.
const sortByField = _fields.find( ( field ) => {
return field.id === view?.sort?.field;
Comment on lines +381 to +396
Copy link
Contributor

Choose a reason for hiding this comment

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

This change moved the computation of sortByField into the sorting function and added groupByField, but both are independent of comparison elements a and b. Similarly, they are computed before checking whether they are needed (view.groupByField and view.sort). Lastly, the sorting is now always performed, even if there aren't the right fields for it.

This should be:

sortByField = view.sort && find()
groupByField = view.groupByField && find()

if (sortByField || groupByField) {
  filteredData.sort((a, b) => {
    if (groupByField) {  }
    if (sortByField) {  }
    return 0
  } )
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed this at #70822

Theoretically, this could have a performance hit in situations where the field to sort/group by wasn't found. In practice, that field is defined by the consumer, or it's the result of users clicking around — unless there's a bug, the provided field will always exist. Is that your assessment as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

unless there's a bug, the provided field will always exist. Is that your assessment as well?

I think so, but I didn't think much of that scenario. I was drawn more to the unnecessary recomputing and sorting. :)

} );
if ( view.sort && sortByField ) {
return sortByField.sort( a, b, view.sort?.direction ?? 'desc' );
}

return 0;
} );
}

// Handle pagination.
Expand Down
61 changes: 61 additions & 0 deletions packages/dataviews/src/test/filter-and-sort-data-view.js
Original file line number Diff line number Diff line change
Expand Up @@ -748,6 +748,45 @@
} );

describe( 'sorting', () => {
it( 'should sort by groupByField first, then by sort.field', () => {
const { data: result } = filterSortAndPaginate(
data,
{
sort: { field: 'title', direction: 'desc' },
groupByField: 'type',
},
fields
);

expect( result ).toHaveLength( 11 );

expect( result[ 0 ].type ).toBe( 'Gas giant' );
expect( result[ 0 ].title ).toBe( 'Saturn' );
expect( result[ 1 ].type ).toBe( 'Gas giant' );
expect( result[ 1 ].title ).toBe( 'Jupiter' );

expect( result[ 2 ].type ).toBe( 'Ice giant' );
expect( result[ 2 ].title ).toBe( 'Uranus' );
expect( result[ 3 ].type ).toBe( 'Ice giant' );
expect( result[ 3 ].title ).toBe( 'Neptune' );

expect( result[ 4 ].type ).toBe( 'Not a planet' );

Check failure on line 773 in packages/dataviews/src/test/filter-and-sort-data-view.js

View workflow job for this annotation

GitHub Actions / JavaScript (Node.js 22) 4/4

Error: expect(received).toBe(expected) // Object.is equality Expected: "Not a planet" Received: "Satellite" at Object.toBe (/home/runner/work/gutenberg/gutenberg/packages/dataviews/src/test/filter-and-sort-data-view.js:773:30) at Promise.then.completed (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/utils.js:300:28) at new Promise (<anonymous>) at callAsyncCircusFn (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/utils.js:233:10) at _callCircusTest (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/run.js:315:40) at processTicksAndRejections (node:internal/process/task_queues:105:5) at _runTest (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/run.js:251:3) at _runTestsForDescribeBlock (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/run.js:125:9) at _runTestsForDescribeBlock (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/run.js:120:9) at run (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/run.js:70:3) at runAndTransformResultsToJestFormat (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:122:21) at jestAdapter (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:79:19) at runTestInternal (/home/runner/work/gutenberg/gutenberg/node_modules/jest-runner/build/runTest.js:367:16) at runTest (/home/runner/work/gutenberg/gutenberg/node_modules/jest-runner/build/runTest.js:444:34) at Object.worker (/home/runner/work/gutenberg/gutenberg/node_modules/jest-runner/build/testWorker.js:106:12)

Check failure on line 773 in packages/dataviews/src/test/filter-and-sort-data-view.js

View workflow job for this annotation

GitHub Actions / JavaScript (Node.js 20) 4/4

Error: expect(received).toBe(expected) // Object.is equality Expected: "Not a planet" Received: "Satellite" at Object.toBe (/home/runner/work/gutenberg/gutenberg/packages/dataviews/src/test/filter-and-sort-data-view.js:773:30) at Promise.then.completed (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/utils.js:300:28) at new Promise (<anonymous>) at callAsyncCircusFn (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/utils.js:233:10) at _callCircusTest (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/run.js:315:40) at processTicksAndRejections (node:internal/process/task_queues:95:5) at _runTest (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/run.js:251:3) at _runTestsForDescribeBlock (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/run.js:125:9) at _runTestsForDescribeBlock (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/run.js:120:9) at run (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/run.js:70:3) at runAndTransformResultsToJestFormat (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:122:21) at jestAdapter (/home/runner/work/gutenberg/gutenberg/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:79:19) at runTestInternal (/home/runner/work/gutenberg/gutenberg/node_modules/jest-runner/build/runTest.js:367:16) at runTest (/home/runner/work/gutenberg/gutenberg/node_modules/jest-runner/build/runTest.js:444:34) at Object.worker (/home/runner/work/gutenberg/gutenberg/node_modules/jest-runner/build/testWorker.js:106:12)
expect( result[ 4 ].title ).toBe( 'Space' );
expect( result[ 5 ].type ).toBe( 'Not a planet' );
expect( result[ 5 ].title ).toBe( 'NASA' );
expect( result[ 6 ].type ).toBe( 'Not a planet' );
expect( result[ 6 ].title ).toBe( 'Apollo' );

expect( result[ 7 ].type ).toBe( 'Terrestrial' );
expect( result[ 7 ].title ).toBe( 'Venus' );
expect( result[ 8 ].type ).toBe( 'Terrestrial' );
expect( result[ 8 ].title ).toBe( 'Mercury' );
expect( result[ 9 ].type ).toBe( 'Terrestrial' );
expect( result[ 9 ].title ).toBe( 'Mars' );
expect( result[ 10 ].type ).toBe( 'Terrestrial' );
expect( result[ 10 ].title ).toBe( 'Earth' );
} );

it( 'should sort integer field types', () => {
const { data: result } = filterSortAndPaginate(
data,
Expand Down Expand Up @@ -853,6 +892,28 @@
expect( result[ 0 ].title ).toBe( 'Uranus' );
expect( result[ 1 ].title ).toBe( 'Neptune' );
} );

it( 'should sort only by groupByField when sort is not specified', () => {
const { data: result } = filterSortAndPaginate(
data,
{
groupByField: 'type',
},
fields
);

let currentType = result[ 0 ].type;
let groupCount = 1;

for ( let i = 1; i < result.length; i++ ) {
if ( result[ i ].type !== currentType ) {
currentType = result[ i ].type;
groupCount++;
}
}

expect( groupCount ).toBe( 4 );
} );
} );

describe( 'pagination', () => {
Expand Down
5 changes: 5 additions & 0 deletions packages/dataviews/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,11 @@ interface ViewBase {
* Whether to show the hierarchical levels.
*/
showLevels?: boolean;

/**
* The field to group by.
*/
groupByField?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Any thoughts on a UI to configure this? (not necessary for this PR)

Also would be good to document this in the README.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done at d97929e

Copy link
Member Author

Choose a reason for hiding this comment

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

Any thoughts on a UI to configure this? (not necessary for this PR)

The obvious way is to add a new select in the view config somewhere. Alternatively, we could explore making it part of the field list (similarly to what we have for the media field).

}

export interface ColumnStyle {
Expand Down
Loading