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

Add flag to Column to sort nulls to the bottom #3506

Merged
merged 6 commits into from
Oct 25, 2023
Merged

Add flag to Column to sort nulls to the bottom #3506

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e69b05a
Add flag to Column to sort nulls to the bottom
TomTirapani Oct 18, 2023
fbfe910
Replace `bottomNullSort` with `sortToBottom`
TomTirapani Oct 23, 2023
7420659
Merge branch 'develop' into nullSort
TomTirapani Oct 25, 2023
e073581
Move sortToBottom implementation to be internal to Column, leaving th…
TomTirapani Oct 25, 2023
96ceb47
Fix CHANGELOG
TomTirapani Oct 25, 2023
33921bc
Simplifiactions to sortToBottom
lbwexler Oct 25, 2023
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
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@

## 60.0.0-SNAPSHOT - unreleased

### 🎁 New Features

* Added `Column.bottomNullSort` - when true, columms will always sort null values to the bottom,
regardless of sort direction.

### 🐞 Bug Fixes
* Fix to pass correct arguments to `ErrorMessageProps.actionFn` and `ErrorMessageProps.detailsFn`
* Better default error text in `ErrorMessage`
Expand Down
10 changes: 5 additions & 5 deletions cmp/grid/GridSorter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,19 +49,19 @@ export class GridSorter {
}

/** Comparator to use with instances of GridSorter.*/
comparator(v1, v2) {
comparator(v1: any, v2: any, bottomNull: boolean) {
if (this.abs) {
v1 = isNumber(v1) ? Math.abs(v1) : v1;
v2 = isNumber(v2) ? Math.abs(v2) : v2;
}
return GridSorter.defaultComparator(v1, v2);
return GridSorter.defaultComparator(v1, v2, bottomNull, this.sort);
}

/** Static comparator to use when a GridSorter instance is not available.*/
static defaultComparator(v1, v2) {
static defaultComparator(v1: any, v2: any, bottomNull: boolean, sortDir: 'asc' | 'desc') {
if (isNil(v1) && isNil(v2)) return 0;
if (isNil(v1)) return -1;
if (isNil(v2)) return 1;
if (isNil(v1)) return bottomNull && sortDir === 'asc' ? 1 : -1;
if (isNil(v2)) return bottomNull && sortDir === 'asc' ? -1 : 1;

if (v1.toNumber) v1 = v1.toNumber();
if (v2.toNumber) v2 = v2.toNumber();
Expand Down
34 changes: 28 additions & 6 deletions cmp/grid/columns/Column.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,11 @@ export interface ColumnSpec {
*/
absSort?: boolean;

/**
* True to always sort null / undefined values to the bottom, regardless of sort order.
*/
bottomNullSort?: boolean;

/**
Copy link
Member

Choose a reason for hiding this comment

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

Would we want to extend this to NaN, '', undefined? We should consider which other sentinel values (other than zero) might warrant the same treatment? We could even have a list of values, that should be put at the bottom, with the order they should appear there. Would this be useful for values like 'CLOSED'? Or other high-level ordering we want to impose independent of the sorting.

Copy link
Member

Choose a reason for hiding this comment

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

could be something like sortToTop:[] sortToBottom:[]

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 really like this idea - makes it more flexible and generalizable. We could support an array of values of equality checks, or boolean closures for more complex checks (e.g. v => isNil(v) || itEmpty(v))

Copy link
Member

Choose a reason for hiding this comment

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

For values like 'CLOSED', user expectations have been that the 'CLOSED' record be at the bottom no matter which column is sorted, and no matter which column has the 'CLOSED'. So, not sure how useful this would be, unless the comparators allow getting the value from other fields.

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, I don't think we can use this for cross-column sorting like you describe. I think that's kind of an edge case and not necessarily something we should support with hoist out of the box, where the 'normal' use case for sorting is only concerned with the sorted column.

Copy link
Member

Choose a reason for hiding this comment

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

Am concerned with the API change on what gets passed to default comparator -- wondering if the "sortToBottom" setting should just translate to a new comparator, or a wrapping comparator around whatever comparator is provided. If the former, a warning could be triggered if you supplied both 'sortToBottom' and comparator and one of them would be ignored.

Copy link
Member

@lbwexler lbwexler Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@TomTirapani -- simplified slightly, tweaked performance, and also allowed it to take a single value.

Let me know if it makes sense?

* Alternate field name to reference or function to call when producing a value for this column
* to be sorted by.
Expand Down Expand Up @@ -433,6 +438,7 @@ export class Column {
rowHeight: number;
sortingOrder: ColumnSortSpec[];
absSort: boolean;
bottomNullSort: boolean;
sortValue: string | ColumnSortValueFn;
comparator: ColumnComparator;
resizable: boolean;
Expand Down Expand Up @@ -504,6 +510,7 @@ export class Column {
flex,
rowHeight,
absSort,
bottomNullSort,
sortingOrder,
sortValue,
comparator,
Expand Down Expand Up @@ -598,6 +605,7 @@ export class Column {
this.rowHeight = rowHeight;

this.absSort = withDefault(absSort, false);
this.bottomNullSort = withDefault(bottomNullSort, false);
this.sortingOrder = this.parseSortingOrder(sortingOrder);
this.sortValue = sortValue;
this.comparator = comparator;
Expand Down Expand Up @@ -897,18 +905,23 @@ export class Column {
if (this.comparator === undefined) {
// Use default comparator with appropriate inputs
ret.comparator = (valueA, valueB, agNodeA, agNodeB) => {
const recordA = agNodeA?.data,
const {gridModel, colId, bottomNullSort} = this,
// Note: sortCfg and agNodes can be undefined if comparator called during show
// of agGrid column header set filter menu.
sortCfg = find(gridModel.sortBy, {colId}),
sortDir = sortCfg?.sort || 'asc',
recordA = agNodeA?.data,
recordB = agNodeB?.data;

valueA = this.getSortValue(valueA, recordA);
valueB = this.getSortValue(valueB, recordB);

return this.defaultComparator(valueA, valueB);
return this.defaultComparator(valueA, valueB, bottomNullSort, sortDir);
};
} else {
// ...or process custom comparator with the Hoist-defined comparatorFn API.
ret.comparator = (valueA, valueB, agNodeA, agNodeB) => {
const {gridModel, colId} = this,
const {gridModel, colId, bottomNullSort} = this,
// Note: sortCfg and agNodes can be undefined if comparator called during show
// of agGrid column header set filter menu.
sortCfg = find(gridModel.sortBy, {colId}),
Expand All @@ -921,7 +934,9 @@ export class Column {
recordB,
column: this,
gridModel,
defaultComparator: (a, b) => this.defaultComparator(a, b),
defaultComparator: (a, b) => {
return this.defaultComparator(a, b, bottomNullSort, sortDir);
},
agNodeA,
agNodeB
};
Expand Down Expand Up @@ -974,9 +989,16 @@ export class Column {
// Implementation
//--------------------
// Default comparator sorting to absValue-aware GridSorters in GridModel.sortBy[].
private defaultComparator = (v1, v2) => {
private defaultComparator = (
v1: any,
v2: any,
bottomNull: boolean,
sortDir: 'asc' | 'desc'
) => {
const sortCfg = find(this.gridModel.sortBy, {colId: this.colId});
return sortCfg ? sortCfg.comparator(v1, v2) : GridSorter.defaultComparator(v1, v2);
return sortCfg
? sortCfg.comparator(v1, v2, bottomNull)
: GridSorter.defaultComparator(v1, v2, bottomNull, sortDir);
};

private defaultSetValueFn = ({value, record, store, field}) => {
Expand Down