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.

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

DataViews: sort author by name + allow custom sort function #64064

Merged
merged 9 commits into from
Aug 1, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
17 changes: 17 additions & 0 deletions packages/dataviews/src/components/dataviews/stories/fixtures.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export const data = [
type: 'Not a planet',
categories: [ 'Space', 'NASA' ],
satellites: 0,
satellites_no_type: 0,
},
{
id: 2,
Expand All @@ -32,6 +33,7 @@ export const data = [
type: 'Not a planet',
categories: [ 'Space' ],
satellites: 0,
satellites_no_type: 0,
},
{
id: 3,
Expand All @@ -41,6 +43,7 @@ export const data = [
type: 'Not a planet',
categories: [ 'NASA' ],
satellites: 0,
satellites_no_type: 0,
},
{
id: 4,
Expand All @@ -50,6 +53,7 @@ export const data = [
type: 'Ice giant',
categories: [ 'Space', 'Planet', 'Solar system' ],
satellites: 14,
satellites_no_type: 14,
},
{
id: 5,
Expand All @@ -59,6 +63,7 @@ export const data = [
type: 'Terrestrial',
categories: [ 'Space', 'Planet', 'Solar system' ],
satellites: 0,
satellites_no_type: 0,
},
{
id: 6,
Expand All @@ -68,6 +73,7 @@ export const data = [
type: 'Terrestrial',
categories: [ 'Space', 'Planet', 'Solar system' ],
satellites: 0,
satellites_no_type: 0,
},
{
id: 7,
Expand All @@ -77,6 +83,7 @@ export const data = [
type: 'Terrestrial',
categories: [ 'Space', 'Planet', 'Solar system' ],
satellites: 1,
satellites_no_type: 1,
},
{
id: 8,
Expand All @@ -86,6 +93,7 @@ export const data = [
type: 'Terrestrial',
categories: [ 'Space', 'Planet', 'Solar system' ],
satellites: 2,
satellites_no_type: 2,
},
{
id: 9,
Expand All @@ -95,6 +103,7 @@ export const data = [
type: 'Gas giant',
categories: [ 'Space', 'Planet', 'Solar system' ],
satellites: 95,
satellites_no_type: 95,
},
{
id: 10,
Expand All @@ -104,6 +113,7 @@ export const data = [
type: 'Gas giant',
categories: [ 'Space', 'Planet', 'Solar system' ],
satellites: 146,
satellites_no_type: 146,
},
{
id: 11,
Expand All @@ -113,6 +123,7 @@ export const data = [
type: 'Ice giant',
categories: [ 'Space', 'Ice giant', 'Solar system' ],
satellites: 28,
satellites_no_type: 28,
},
];

Expand Down Expand Up @@ -189,6 +200,12 @@ export const fields = [
{
label: 'Satellites',
id: 'satellites',
type: 'integer',
enableSorting: true,
},
{
label: 'Satellites (no type)',
id: 'satellites_no_type',
enableSorting: true,
},
{
Expand Down
22 changes: 22 additions & 0 deletions packages/dataviews/src/field-types/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/**
* Internal dependencies
*/
import { default as integer } from './integer';
import type { FieldType } from '../types';

/**
*
* @param {FieldType} type The field type definition to get.
*
* @return A field type definition.
*/
export default function getFieldTypeDefinition( type?: FieldType ) {
if ( 'integer' === type ) {
return integer;
}

// If no type found, the sort function doesn't do anything.
Copy link
Member Author

Choose a reason for hiding this comment

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

What do we think of this? What do we do if the type is not found?

return {
sort: () => 0,
};
}
22 changes: 22 additions & 0 deletions packages/dataviews/src/field-types/integer.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice file :)

Should we move the "control" to the field types definition?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes! And validation, etc. I'll do that in a follow-up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Validation moved to the type definition at #64164

* Internal dependencies
*/
import type { SortDirection } from '../types';

function sort< Item >(
a: {
item: Item;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have Item here at all? I was thinking a: any, b: any should be sufficient no?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you did, I think you're misunderstanding what I'm suggesting. I'm suggesting that fieldType.sort and field.sort to have different APIs. one receives the items, the other receives the values. Do you want me to try to commit what I have in mind?

Copy link
Member Author

@oandregal oandregal Jul 31, 2024

Choose a reason for hiding this comment

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

@youknowriad please, be welcome to push. In my mind both functions are instances of the same idea, so they'd have the same signature. But I'm open to try different things: push and we can discuss what's there.

Copy link
Contributor

Choose a reason for hiding this comment

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

For me field types and fields work at different levels, field types work on values, fields work on items, so it doesn't make sense to have the same API.

I've pushed what I had in mind in 621ac69

Feel free to revert if you don't like 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.

621ac69 works for me.

value: any;
},
b: {
item: Item;
value: any;
},
direction: SortDirection
) {
return direction === 'asc' ? a.value - b.value : b.value - a.value;
}

export default {
sort,
};
15 changes: 15 additions & 0 deletions packages/dataviews/src/filter-and-sort-data-view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,21 @@ export function filterSortAndPaginate< Item >(
const valueA = fieldToSort.getValue( { item: a } ) ?? '';
const valueB = fieldToSort.getValue( { item: b } ) ?? '';

if ( fieldToSort.type === 'integer' ) {
return fieldToSort.sort(
oandregal marked this conversation as resolved.
Show resolved Hide resolved
{
item: a,
value: valueA,
},
{
item: b,
value: valueB,
},
view.sort?.direction ?? 'desc'
);
}

// When/if types become required, we can remove the following logic.
if (
typeof valueA === 'number' &&
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that I've left the existing logic here because type is optional. When/If make it required this could be removed.

typeof valueB === 'number'
Expand Down
6 changes: 6 additions & 0 deletions packages/dataviews/src/normalize-fields.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/**
* Internal dependencies
*/
import getFieldTypeDefinition from './field-types';
import type { Field, NormalizedField, ItemRecord } from './types';

/**
Expand All @@ -13,15 +14,20 @@ export function normalizeFields< Item >(
fields: Field< Item >[]
): NormalizedField< Item >[] {
return fields.map( ( field ) => {
const fieldTypeDefinition = getFieldTypeDefinition( field.type );

const getValue =
field.getValue ||
( ( { item }: { item: ItemRecord } ) => item[ field.id ] );

const sort = field.sort || fieldTypeDefinition.sort;

return {
...field,
label: field.label || field.id,
getValue,
render: field.render || getValue,
sort,
};
} );
}
15 changes: 15 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 @@ -254,6 +254,21 @@ describe( 'sorting', () => {
} );

it( 'should sort by number', () => {
const { data: result } = filterSortAndPaginate(
data,
{
sort: { field: 'satellites_no_type', direction: 'desc' },
},
fields
);

expect( result ).toHaveLength( 11 );
expect( result[ 0 ].title ).toBe( 'Saturn' );
expect( result[ 1 ].title ).toBe( 'Jupiter' );
expect( result[ 2 ].title ).toBe( 'Uranus' );
} );

it( 'should sort by type integer', () => {
const { data: result } = filterSortAndPaginate(
data,
{
Expand Down
26 changes: 26 additions & 0 deletions packages/dataviews/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,21 @@ export type Field< Item > = {
*/
render?: ComponentType< { item: Item } >;

/**
* Callback used to sort the field.
*/
sort?: (
a: {
item: Item;
value: any;
},
b: {
item: Item;
value: any;
},
direction: SortDirection
) => number;

/**
* Whether the field is sortable.
*/
Expand Down Expand Up @@ -124,6 +139,17 @@ export type NormalizedField< Item > = Field< Item > & {
label: string;
getValue: ( args: { item: Item } ) => any;
render: ComponentType< { item: Item } >;
sort: (
a: {
item: Item;
value: any;
},
b: {
item: Item;
value: any;
},
direction: SortDirection
) => number;
};

/**
Expand Down
8 changes: 8 additions & 0 deletions packages/edit-site/src/components/post-fields/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,14 @@ function usePostFields( viewType ) {
label: name,
} ) ) || [],
render: PostAuthorField,
sort: ( { item: a }, { item: b }, direction ) => {
const nameA = a._embedded?.author?.[ 0 ]?.name || '';
const nameB = b._embedded?.author?.[ 0 ]?.name || '';

return direction === 'asc'
? nameA.localeCompare( nameB )
: nameB.localeCompare( nameA );
},
},
{
label: __( 'Status' ),
Expand Down
26 changes: 20 additions & 6 deletions packages/edit-site/src/components/post-list/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
import { useState, useMemo, useCallback, useEffect } from '@wordpress/element';
import { privateApis as routerPrivateApis } from '@wordpress/router';
import { useSelect, useDispatch } from '@wordpress/data';
import { DataViews } from '@wordpress/dataviews';
import { DataViews, filterSortAndPaginate } from '@wordpress/dataviews';
import { privateApis as editorPrivateApis } from '@wordpress/editor';
import { __ } from '@wordpress/i18n';
import { drawerRight } from '@wordpress/icons';
Expand Down Expand Up @@ -161,6 +161,8 @@ export default function PostList( { postType } ) {
[ history ]
);

const { isLoading: isLoadingFields, fields } = usePostFields( view.type );

const queryArgs = useMemo( () => {
const filters = {};
view.filters.forEach( ( filter ) => {
Expand Down Expand Up @@ -200,12 +202,25 @@ export default function PostList( { postType } ) {
}, [ view ] );
const {
records,
isResolving: isLoadingMainEntities,
isResolving: isLoadingData,
totalItems,
totalPages,
} = useEntityRecordsWithPermissions( 'postType', postType, queryArgs );

const ids = records?.map( ( record ) => getItemId( record ) ) ?? [];
// The REST API sort the authors by ID, but we want to sort them by name.
const data = useMemo( () => {
if ( ! isLoadingFields && view?.sort?.field === 'author' ) {
return filterSortAndPaginate(
records,
{ sort: { ...view.sort } },
fields
).data;
}

return records;
}, [ records, fields, isLoadingFields, view?.sort ] );

const ids = data?.map( ( record ) => getItemId( record ) ) ?? [];
const prevIds = usePrevious( ids ) ?? [];
const deletedIds = prevIds.filter( ( id ) => ! ids.includes( id ) );
const postIdWasDeleted = deletedIds.includes( postId );
Expand Down Expand Up @@ -263,7 +278,6 @@ export default function PostList( { postType } ) {
} );
closeModal();
};
const { isLoading: isLoadingFields, fields } = usePostFields( view.type );

return (
<Page
Expand Down Expand Up @@ -294,8 +308,8 @@ export default function PostList( { postType } ) {
paginationInfo={ paginationInfo }
fields={ fields }
actions={ actions }
data={ records || EMPTY_ARRAY }
isLoading={ isLoadingMainEntities || isLoadingFields }
data={ data || EMPTY_ARRAY }
isLoading={ isLoadingData || isLoadingFields }
view={ view }
onChangeView={ setView }
selection={ selection }
Expand Down
Loading