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

[DataGrid] Enhance comparator signature to support advanced use cases #7406

Merged
merged 9 commits into from
Dec 13, 2023
1 change: 1 addition & 0 deletions changelogs/upcoming/7406.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Updated `EuiDataGridSchemaDetector`'s comparator arguments to include entry indexes
11 changes: 8 additions & 3 deletions src/components/datagrid/data_grid_types.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

I know the PR has already merged, but just curious - from a developer experience perspective, do you think the type/prop docs are sufficient, or should we add/update an existing EuiDataGrid documentation example as well?

Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,14 @@ export interface EuiDataGridSchemaDetector {
*/
detector: (value: string) => number;
/**
* A custom comparator function when performing in-memory sorting on this data type, takes `(a: string, b: string, direction: 'asc' | 'desc) => -1 | 0 | 1`
*/
comparator?: (a: string, b: string, direction: 'asc' | 'desc') => -1 | 0 | 1;
* A custom comparator function when performing in-memory sorting on this data type, takes `(a: string, b: string, direction: 'asc' | 'desc', indexes: {aIndex: number, bIndex: number}) => -1 | 0 | 1`
*/
comparator?: (
a: string,
b: string,
direction: 'asc' | 'desc',
indexes: { aIndex: number; bIndex: number }
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I might actually open a new PR for this. I think we should have typed this as

Suggested change
indexes: { aIndex: number; bIndex: number }
optional?: { aIndex: number; bIndex: number }

to make it clearer it's extra information that's optional for developer usage

Copy link
Member

Choose a reason for hiding this comment

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

🤦 Jumped the gun on the merge, my bad!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a developer I read that as optional can either be passed or not, please check.
I think it make sense the renaming (indexes => optional) but the optional type (optional?: )is actually misleading there. It makes more sense to keep it without the optional to communicate to the developer that he might use it or not, but it will always be passed.

A similar approach is used also for all JS Array methods, i.e. Array.prototype.filter:

filter(predicate: (value: T, index: number, array: T[]) => unknown, thisArg?: any): T[];

The array value there is not often used, but it will be always passed no matter what predicate function is defined. The optional type is applied only to the thisArg, as it can be available depending on the actual usage.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh yeah that's a super good point about the ts ?, thanks for catching that! Just renaming the arg to optional for documentation & future flexibility should be fine in that case

) => -1 | 0 | 1;
/**
* The icon used to visually represent this data type. Accepts any `EuiIcon IconType`.
*/
Expand Down
59 changes: 59 additions & 0 deletions src/components/datagrid/utils/sorting.test.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for creating this test file! ❤️ You rock Marco, the test looks really great!

Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { renderHook } from '@testing-library/react';
import { useSorting, type useSortingArgs } from './sorting';

function getDefaultArgs(): useSortingArgs {
return {
sorting: {
columns: [{ id: '1', direction: 'asc' }],
onSort: jest.fn(),
},
schema: { '1': { columnType: 'number' } },
schemaDetectors: [],
startRow: 0,
inMemoryValues: { '0': { '1': '0' }, '1': { '1': '1' } },
inMemory: { level: 'sorting' },
};
}

function testUseSorting(args: useSortingArgs) {
return renderHook(() => useSorting(args)).result.current;
}

describe('useSorting', () => {
it('returns null if no sorting object is provided', () => {
expect(
testUseSorting({ ...getDefaultArgs(), schema: {}, inMemoryValues: {} })
.sortedRowMap
).toEqual([]);
});

it('should pass all arguments to custom comparator fn', () => {
const customComparator = jest.fn();
testUseSorting({
...getDefaultArgs(),
schemaDetectors: [
{
type: 'number',
detector: () => 1,
comparator: customComparator,
icon: 'empty',
sortTextAsc: 'ASC',
sortTextDesc: 'DESC',
},
],
});

expect(customComparator).toHaveBeenCalledWith('1', '0', 'asc', {
aIndex: 1,
bIndex: 0,
});
});
});
23 changes: 14 additions & 9 deletions src/components/datagrid/utils/sorting.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,21 +24,23 @@ export const DataGridSortingContext =
getCorrectRowIndex: (number) => number,
});

export type useSortingArgs = {
sorting?: EuiDataGridSorting;
inMemory?: EuiDataGridInMemory;
inMemoryValues: EuiDataGridInMemoryValues;
schema: EuiDataGridSchema;
schemaDetectors: EuiDataGridSchemaDetector[];
startRow: number;
};

export const useSorting = ({
sorting,
inMemory,
inMemoryValues,
schema,
schemaDetectors,
startRow,
}: {
sorting?: EuiDataGridSorting;
inMemory?: EuiDataGridInMemory;
inMemoryValues: EuiDataGridInMemoryValues;
schema: EuiDataGridSchema;
schemaDetectors: EuiDataGridSchemaDetector[];
startRow: number;
}) => {
}: useSortingArgs) => {
const sortingColumns = sorting?.columns;

const sortedRowMap = useMemo(() => {
Expand Down Expand Up @@ -80,7 +82,10 @@ export const useSorting = ({
}
}

const result = comparator(aValue, bValue, column.direction);
const result = comparator(aValue, bValue, column.direction, {
aIndex: a.index,
bIndex: b.index,
});
// only return if the columns are unequal, otherwise allow the next sort-by column to run
if (result !== 0) return result;
}
Expand Down
Loading