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

[DataGrid] Fix valueGetter sorting #348

Merged
merged 9 commits into from
Sep 24, 2020
Merged

[DataGrid] Fix valueGetter sorting #348

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8a10c75
fix valueGetter sorting
dtassone Sep 24, 2020
54a9022
fix eslint
dtassone Sep 24, 2020
28900e8
fix prettier
dtassone Sep 24, 2020
6b9b70f
Apply suggestions from code review
dtassone Sep 24, 2020
f714ea0
Update docs/src/pages/components/data-grid/rendering/ValueGetterGrid.tsx
dtassone Sep 24, 2020
0c931c2
fix reviews
dtassone Sep 24, 2020
fc48512
Update docs/src/pages/components/data-grid/rendering/rendering.md
oliviertassinari Sep 24, 2020
775816b
prettier
dtassone Sep 24, 2020
e1fcbda
fix tsx format in demo
dtassone Sep 24, 2020
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
11 changes: 7 additions & 4 deletions docs/src/pages/components/data-grid/rendering/ValueGetterGrid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import * as React from 'react';
import { DataGrid } from '@material-ui/data-grid';

// You can optimize the perf by memoizing this function.
const getFullName = (params) =>
`${params.getValue('firstName') || ''} ${params.getValue('lastName') || ''}`;

const columns = [
{ field: 'id', hide: true },
{ field: 'firstName', headerName: 'First name', width: 130 },
Expand All @@ -9,10 +13,9 @@ const columns = [
field: 'fullName',
headerName: 'Full name',
width: 160,
valueGetter: (params) =>
`${params.getValue('firstName') || ''} ${
params.getValue('lastName') || ''
}`,
valueGetter: getFullName,
sortComparator: (v1, v2, cellParams1, cellParams2) =>
getFullName(cellParams1).localeCompare(getFullName(cellParams2)),
},
];

Expand Down
11 changes: 7 additions & 4 deletions docs/src/pages/components/data-grid/rendering/ValueGetterGrid.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import * as React from 'react';
import { DataGrid, ColDef, ValueGetterParams } from '@material-ui/data-grid';

// You can optimize the perf by memoizing this function.
const getFullName = (params: ValueGetterParams) =>
`${params.getValue('firstName') || ''} ${params.getValue('lastName') || ''}`;
Copy link
Member

@oliviertassinari oliviertassinari Sep 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think that we have to stick to a convention between arrow functions and named function. The current one is named function when possible, e.g. for the top-level scope and arrow functions for nested scopes, when inlining is simpler.

Suggested change
const getFullName = (params: ValueGetterParams) =>
`${params.getValue('firstName') || ''} ${params.getValue('lastName') || ''}`;
function getFullName(params: ValueGetterParams) {
return `${params.getValue('firstName') || ''} ${params.getValue('lastName') || ''}`;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Up to you. I prefer the simple and concise syntax of arrow functions.

Copy link
Member

@oliviertassinari oliviertassinari Sep 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, time for a conversation about it then. @mui-org/core-team.

The problem:
There are two ways to write functions: arrow or named. Defining one convention could avoid noise during refactorization (depending on who writes which line), increase consistency (easier to browse codebase) and avoid having to discuss it in reviews.

The question to answer:
When should we use named function vs arrow function? Should we even care? Meaning leaving it up to what the person prefers to be when he writes a line of code or refactor one.
For instance, right now, the styles object is always an arrow function when components are always (or almost) a function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well the main problem of named function is that they lose the context. And the syntax is more verbose.
If you define a class in TS, then it's better to use named function as they get attached to the prototype.
if it's a function component then arrow functions are fine.

I think it's important to be consistent.

Copy link
Member

Choose a reason for hiding this comment

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

If it doesn't matter we shouldn't enforce either style if it's not auto-fixable. If a lint rule can find and fix it we can configure it however you like. But I don't want to argue every time why I prefer one over the other and I don't want reviewers wasting their time nitpicking code style. There's enough substance to review.

Copy link
Member

Choose a reason for hiding this comment

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

And the syntax is more verbose.

Only if you use implicit return. Once you have to use explicit return (and this is something I end up using anyway once you have to debug) the function declaration actually requires one less character.

Another type related downside: const arrow function expressions can't be generic.

I think it's fairly obvious that there isn't a rule to use one or the other. Eventually you have to use function declarations anyway so why disallow them?

Copy link
Member

@oliviertassinari oliviertassinari Sep 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, so we are leaning toward: no rules, use whatever you find best in the given context?


const columns: ColDef[] = [
{ field: 'id', hide: true },
{ field: 'firstName', headerName: 'First name', width: 130 },
Expand All @@ -9,10 +13,9 @@ const columns: ColDef[] = [
field: 'fullName',
headerName: 'Full name',
width: 160,
valueGetter: (params: ValueGetterParams) =>
`${params.getValue('firstName') || ''} ${
params.getValue('lastName') || ''
}`,
valueGetter: getFullName,
sortComparator: (v1, v2, cellParams1, cellParams2) =>
getFullName(cellParams1).localeCompare(getFullName(cellParams2)),
},
];

Expand Down
16 changes: 11 additions & 5 deletions docs/src/pages/components/data-grid/rendering/rendering.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,16 @@ This section is an extension of the main [column definitions documentation](/com

### Value getter

Sometimes a column might not have a corresponding value and you just want to render a combination of different fields. To do that, you can set the `valueGetter` attribute of `ColDef` as in the example below:
Sometimes a column might not have a corresponding value and you just want to render a combination of different fields.
To do that, you can set the `valueGetter` attribute of `ColDef` as in the example below:

<b>Note</b>: You need to set a `sortComparator` for the column sorting to work when setting the `valueGetter` attribute.

```tsx
// You can optimize the perf by memoizing this function.
const getFullName = (params: ValueGetterParams) =>
`${params.getValue('firstName') || ''} ${params.getValue('lastName') || ''}`;

const columns: ColDef[] = [
{ field: 'id', hide: true },
{ field: 'firstName', headerName: 'First name', width: 130 },
Expand All @@ -24,10 +31,9 @@ const columns: ColDef[] = [
field: 'fullName',
headerName: 'Full name',
width: 160,
valueGetter: (params: ValueGetterParams) =>
`${params.getValue('firstName') || ''} ${
params.getValue('lastName') || ''
}`,
valueGetter: getFullName,
sortComparator: (v1, v2, cellParams1, cellParams2) =>
getFullName(cellParams1).localeCompare(getFullName(cellParams2)),
},
];
```
Expand Down
43 changes: 32 additions & 11 deletions packages/grid/_modules_/grid/hooks/features/useSorting.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
RowModel,
RowsProp,
SortApi,
CellParams,
} from '../../models';
import {
COLUMN_HEADER_CLICK,
Expand All @@ -21,7 +22,7 @@ import {
SORT_MODEL_CHANGE,
} from '../../constants/eventsConstants';
import { useLogger } from '../utils';
import { isDesc, nextSortDirection } from '../../utils';
import { buildCellParams, isDesc, nextSortDirection } from '../../utils';
import { SortItem, SortModel } from '../../models/sortModel';
import { useApiEventHandler } from '../root/useApiEventHandler';
import { useApiMethod } from '../root/useApiMethod';
Expand Down Expand Up @@ -74,22 +75,42 @@ export const useSorting = (
[options.sortingOrder],
);

const comparatorListAggregate = React.useCallback((row1: RowModel, row2: RowModel) => {
const result = comparatorList.current.reduce((res, colComparator) => {
const { field, comparator } = colComparator;
res = res || comparator(row1.data[field], row2.data[field], row1, row2);
return res;
}, 0);
return result;
}, []);
const comparatorListAggregate = React.useCallback(
(row1: RowModel, row2: RowModel) => {
const result = comparatorList.current.reduce((res, colComparator) => {
const { field, comparator } = colComparator;
res =
res ||
comparator(
row1.data[field],
row2.data[field],
buildCellParams({
api: apiRef.current,
colDef: apiRef.current.getColumnFromField(field),
rowModel: row1,
value: row1.data[field],
}),
buildCellParams({
api: apiRef.current,
colDef: apiRef.current.getColumnFromField(field),
rowModel: row2,
value: row2.data[field],
}),
);
return res;
}, 0);
return result;
},
[apiRef],
);

const buildComparatorList = React.useCallback(
(sortModel: SortModel): FieldComparatorList => {
const comparators = sortModel.map((item) => {
const column = apiRef.current.getColumnFromField(item.field);
const comparator = isDesc(item.sort)
? (v1: CellValue, v2: CellValue, row1: RowModel, row2: RowModel) =>
-1 * column.sortComparator!(v1, v2, row1, row2)
? (v1: CellValue, v2: CellValue, cellParams1: CellParams, cellParams2: CellParams) =>
-1 * column.sortComparator!(v1, v2, cellParams1, cellParams2)
: column.sortComparator!;
return { field: column.field, comparator };
});
Expand Down
2 changes: 1 addition & 1 deletion packages/grid/_modules_/grid/models/params/cellParams.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export interface CellParams {
/**
* The row index of the row that the current cell belongs to.
*/
rowIndex: number;
rowIndex?: number;
/**
* ApiRef that let you manipulate the grid.
*/
Expand Down
10 changes: 8 additions & 2 deletions packages/grid/_modules_/grid/models/sortModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { CellValue, RowModel } from './rows';
import { CellValue } from './rows';
import { CellParams } from './params/cellParams';

export type SortDirection = 'asc' | 'desc' | null | undefined;

Expand All @@ -7,7 +8,12 @@ export type FieldComparatorList = { field: string; comparator: ComparatorFn }[];
/**
* The type of the sort comparison function.
*/
export type ComparatorFn = (v1: CellValue, v2: CellValue, row1: RowModel, row2: RowModel) => number;
export type ComparatorFn = (
v1: CellValue,
v2: CellValue,
cellParams1: CellParams,
cellParams2: CellParams,
) => number;

/**
* Object that represents the column sorted data, part of the [[SortModel]].
Expand Down
2 changes: 1 addition & 1 deletion packages/grid/_modules_/grid/utils/paramsUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export function buildCellParams({
}: {
rowModel: RowModel;
colDef: ColDef;
rowIndex: number;
rowIndex?: number;
value: CellValue;
api: GridApi;
element?: HTMLElement;
Expand Down
3 changes: 2 additions & 1 deletion packages/storybook/src/stories/grid-sorting.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,8 @@ export const CustomComparator = () => {
field: 'username',
valueGetter: (params) =>
`${params.getValue('name') || 'unknown'}_${params.getValue('age') || 'x'}`,
sortComparator: (v1, v2, row1, row2) => row1.data.age - row2.data.age,
sortComparator: (v1, v2, cellParams1, cellParams2) =>
cellParams1.data.age - cellParams2.data.age,
sortDirection: 'asc',
width: 150,
};
Expand Down