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

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

Merged
merged 9 commits into from
Aug 1, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f469303
Rename for clarity
oandregal Jul 29, 2024
7f4ecf1
Provide a custom sort for author
oandregal Jul 29, 2024
6e9b21c
Data: provide only sort
oandregal Jul 30, 2024
b1ce0e1
Pass the items to the sort function
oandregal Jul 30, 2024
4460490
Type the items
oandregal Jul 30, 2024
344ca6d
Update comment
oandregal Jul 30, 2024
bcf814a
Pass getValue as a parameter to sort
oandregal Jul 30, 2024
eaa0b63
Pass item & value to sort function
oandregal Jul 30, 2024
621ac69
Simplify Sort API
youknowriad Jul 31, 2024
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
18 changes: 10 additions & 8 deletions packages/dataviews/src/field-types/integer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,17 @@
import type { SortDirection } from '../types';

function sort< Item >(
itemA: Item,
itemB: Item,
direction: SortDirection,
getValue: ( args: { item: Item } ) => any
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 •
edited
Loading

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
) {
const valueA = getValue( { item: itemA } );
const valueB = getValue( { item: itemB } );

return direction === 'asc' ? valueA - valueB : valueB - valueA;
return direction === 'asc' ? a.value - b.value : b.value - a.value;
}

export default {
Expand Down
18 changes: 12 additions & 6 deletions packages/dataviews/src/filter-and-sort-data-view.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,18 +140,24 @@ export function filterSortAndPaginate< Item >(
} );
if ( fieldToSort ) {
filteredData.sort( ( a, b ) => {
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
a,
b,
view.sort?.direction ?? 'desc',
fieldToSort.getValue
{
item: a,
value: valueA,
},
{
item: b,
value: valueB,
},
view.sort?.direction ?? 'desc'
);
}

// When/if types become required, we can remove the following logic.
const valueA = fieldToSort.getValue( { item: a } ) ?? '';
const valueB = fieldToSort.getValue( { item: b } ) ?? '';
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
26 changes: 18 additions & 8 deletions packages/dataviews/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,15 @@ export type Field< Item > = {
* Callback used to sort the field.
*/
sort?: (
a: Item,
b: Item,
direction: SortDirection,
getValue: ( args: { item: Item } ) => any
a: {
item: Item;
value: any;
},
b: {
item: Item;
value: any;
},
direction: SortDirection
) => number;

/**
Expand Down Expand Up @@ -135,10 +140,15 @@ export type NormalizedField< Item > = Field< Item > & {
getValue: ( args: { item: Item } ) => any;
render: ComponentType< { item: Item } >;
sort: (
a: Item,
b: Item,
direction: SortDirection,
getValue: ( args: { item: Item } ) => any
a: {
item: Item;
value: any;
},
b: {
item: Item;
value: any;
},
direction: SortDirection
) => number;
};

Expand Down
6 changes: 3 additions & 3 deletions packages/edit-site/src/components/post-fields/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,9 +242,9 @@ function usePostFields( viewType ) {
label: name,
} ) ) || [],
render: PostAuthorField,
sort: ( fieldA, fieldB, direction ) => {
const nameA = fieldA._embedded?.author?.[ 0 ]?.name || '';
const nameB = fieldB._embedded?.author?.[ 0 ]?.name || '';
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 )
Expand Down
Loading