From 5bb406b296b77292d2191cb92ae0967149c8057b Mon Sep 17 00:00:00 2001 From: Corbin Robb <31329271+corbinrobb@users.noreply.github.com> Date: Mon, 14 Feb 2022 14:42:04 -0700 Subject: [PATCH] fix(sqllab): fix query results sorting (#18666) * add sorting for query results grid and add natural order sort * modify tests for natural/original order state * clean up setState Co-authored-by: Corbin Robb --- .../FilterableTable/FilterableTable.test.tsx | 48 +++++++------- .../FilterableTable/FilterableTable.tsx | 64 +++++++++++++++---- .../FilterableTableStyles.less | 1 + 3 files changed, 75 insertions(+), 38 deletions(-) diff --git a/superset-frontend/src/components/FilterableTable/FilterableTable.test.tsx b/superset-frontend/src/components/FilterableTable/FilterableTable.test.tsx index fad9e8245df22..3cb58ce6d06c9 100644 --- a/superset-frontend/src/components/FilterableTable/FilterableTable.test.tsx +++ b/superset-frontend/src/components/FilterableTable/FilterableTable.test.tsx @@ -116,10 +116,10 @@ describe('FilterableTable sorting - RTL', () => { expect(gridCells[1]).toHaveTextContent('Bravo'); expect(gridCells[2]).toHaveTextContent('Alpha'); - // Third click to sort ascending again + // Third click to clear sorting userEvent.click(stringColumn); - expect(gridCells[0]).toHaveTextContent('Alpha'); - expect(gridCells[1]).toHaveTextContent('Bravo'); + expect(gridCells[0]).toHaveTextContent('Bravo'); + expect(gridCells[1]).toHaveTextContent('Alpha'); expect(gridCells[2]).toHaveTextContent('Charlie'); }); @@ -151,10 +151,10 @@ describe('FilterableTable sorting - RTL', () => { expect(gridCells[1]).toHaveTextContent('10'); expect(gridCells[2]).toHaveTextContent('0'); - // Third click to sort ascending again + // Third click to clear sorting userEvent.click(integerColumn); - expect(gridCells[0]).toHaveTextContent('0'); - expect(gridCells[1]).toHaveTextContent('10'); + expect(gridCells[0]).toHaveTextContent('10'); + expect(gridCells[1]).toHaveTextContent('0'); expect(gridCells[2]).toHaveTextContent('100'); }); @@ -186,10 +186,10 @@ describe('FilterableTable sorting - RTL', () => { expect(gridCells[1]).toHaveTextContent('45.67'); expect(gridCells[2]).toHaveTextContent('1.23'); - // Third click to sort ascending again + // Third click to clear sorting userEvent.click(floatColumn); - expect(gridCells[0]).toHaveTextContent('1.23'); - expect(gridCells[1]).toHaveTextContent('45.67'); + expect(gridCells[0]).toHaveTextContent('45.67'); + expect(gridCells[1]).toHaveTextContent('1.23'); expect(gridCells[2]).toHaveTextContent('89.0000001'); }); @@ -257,17 +257,17 @@ describe('FilterableTable sorting - RTL', () => { expect(gridCells[9]).toHaveTextContent('26260.210000000003'); expect(gridCells[10]).toHaveTextContent('24078.610000000004'); - // Third click to sort ascending again + // Third click to clear sorting userEvent.click(mixedFloatColumn); - expect(gridCells[0]).toHaveTextContent('24078.610000000004'); - expect(gridCells[1]).toHaveTextContent('26260.210000000003'); - expect(gridCells[2]).toHaveTextContent('28550.59'); - expect(gridCells[3]).toHaveTextContent('48710.92'); - expect(gridCells[4]).toHaveTextContent('72212.86'); - expect(gridCells[5]).toHaveTextContent('98089.08000000002'); - expect(gridCells[6]).toHaveTextContent('144729.96000000002'); - expect(gridCells[7]).toHaveTextContent('145776.56'); - expect(gridCells[8]).toHaveTextContent('152718.97999999998'); + expect(gridCells[0]).toHaveTextContent('48710.92'); + expect(gridCells[1]).toHaveTextContent('145776.56'); + expect(gridCells[2]).toHaveTextContent('72212.86'); + expect(gridCells[3]).toHaveTextContent('144729.96000000002'); + expect(gridCells[4]).toHaveTextContent('26260.210000000003'); + expect(gridCells[5]).toHaveTextContent('152718.97999999998'); + expect(gridCells[6]).toHaveTextContent('28550.59'); + expect(gridCells[7]).toHaveTextContent('24078.610000000004'); + expect(gridCells[8]).toHaveTextContent('98089.08000000002'); expect(gridCells[9]).toHaveTextContent('3439718.0300000007'); expect(gridCells[10]).toHaveTextContent('4528047.219999993'); }); @@ -320,14 +320,14 @@ describe('FilterableTable sorting - RTL', () => { expect(gridCells[5]).toHaveTextContent('2021-01-02'); expect(gridCells[6]).toHaveTextContent('2021-01-01'); - // Third click to sort ascending again + // Third click to clear sorting userEvent.click(dsColumn); expect(gridCells[0]).toHaveTextContent('2021-01-01'); - expect(gridCells[1]).toHaveTextContent('2021-01-02'); - expect(gridCells[2]).toHaveTextContent('2021-01-03'); - expect(gridCells[3]).toHaveTextContent('2021-10-01'); + expect(gridCells[1]).toHaveTextContent('2022-01-01'); + expect(gridCells[2]).toHaveTextContent('2021-01-02'); + expect(gridCells[3]).toHaveTextContent('2021-01-03'); expect(gridCells[4]).toHaveTextContent('2021-12-01'); - expect(gridCells[5]).toHaveTextContent('2022-01-01'); + expect(gridCells[5]).toHaveTextContent('2021-10-01'); expect(gridCells[6]).toHaveTextContent('2022-01-02'); }); }); diff --git a/superset-frontend/src/components/FilterableTable/FilterableTable.tsx b/superset-frontend/src/components/FilterableTable/FilterableTable.tsx index 29a40d320d387..23d68f96850c9 100644 --- a/superset-frontend/src/components/FilterableTable/FilterableTable.tsx +++ b/superset-frontend/src/components/FilterableTable/FilterableTable.tsx @@ -114,8 +114,9 @@ interface FilterableTableProps { interface FilterableTableState { sortBy?: string; - sortDirection: SortDirectionType; + sortDirection?: SortDirectionType; fitted: boolean; + displayedList: Datum[]; } export default class FilterableTable extends PureComponent< @@ -175,8 +176,8 @@ export default class FilterableTable extends PureComponent< this.totalTableHeight = props.height; this.state = { - sortDirection: SortDirection.ASC, fitted: false, + displayedList: [...this.list], }; this.container = React.createRef(); @@ -191,7 +192,7 @@ export default class FilterableTable extends PureComponent< } getWidthsForColumns() { - const PADDING = 40; // accounts for cell padding and width of sorting icon + const PADDING = 50; // accounts for cell padding and width of sorting icon const widthsByColumnKey = {}; const cellContent = ([] as string[]).concat( ...this.props.orderedColumnKeys.map(key => { @@ -295,7 +296,31 @@ export default class FilterableTable extends PureComponent< sortBy: string; sortDirection: SortDirectionType; }) { - this.setState({ sortBy, sortDirection }); + let updatedState: FilterableTableState; + + const shouldClearSort = + this.state.sortDirection === SortDirection.DESC && + this.state.sortBy === sortBy; + + if (shouldClearSort) { + updatedState = { + ...this.state, + sortBy: undefined, + sortDirection: undefined, + displayedList: [...this.list], + }; + } else { + updatedState = { + ...this.state, + sortBy, + sortDirection, + displayedList: [...this.list].sort( + this.sortResults(sortBy, sortDirection === SortDirection.DESC), + ), + }; + } + + this.setState(updatedState); } fitTableToWidthIfNeeded() { @@ -362,6 +387,17 @@ export default class FilterableTable extends PureComponent< }; } + sortGrid = (label: string) => { + this.sort({ + sortBy: label, + sortDirection: + this.state.sortDirection === SortDirection.DESC || + this.state.sortBy !== label + ? SortDirection.ASC + : SortDirection.DESC, + }); + }; + renderTableHeader({ dataKey, label, @@ -425,8 +461,14 @@ export default class FilterableTable extends PureComponent< : style.top, }} className={`${className} grid-cell grid-header-cell`} + role="columnheader" + tabIndex={columnIndex} + onClick={() => this.sortGrid(label)} > -
{label}
+ {label} + {this.state.sortBy === label && ( + + )} ); @@ -444,7 +486,7 @@ export default class FilterableTable extends PureComponent< style: React.CSSProperties; }) { const columnKey = this.props.orderedColumnKeys[columnIndex]; - const cellData = this.list[rowIndex][columnKey]; + const cellData = this.state.displayedList[rowIndex][columnKey]; const cellText = this.getCellContent({ cellData, columnKey }); const content = cellData === null ? {cellText} : cellText; @@ -563,19 +605,13 @@ export default class FilterableTable extends PureComponent< rowHeight, } = this.props; - let sortedAndFilteredList = this.list; + let sortedAndFilteredList = this.state.displayedList; // filter list if (filterText) { - sortedAndFilteredList = this.list.filter((row: Datum) => + sortedAndFilteredList = sortedAndFilteredList.filter((row: Datum) => this.hasMatch(filterText, row), ); } - // sort list - if (sortBy) { - sortedAndFilteredList = sortedAndFilteredList.sort( - this.sortResults(sortBy, sortDirection === SortDirection.DESC), - ); - } let { height } = this.props; let totalTableHeight = height; diff --git a/superset-frontend/src/components/FilterableTable/FilterableTableStyles.less b/superset-frontend/src/components/FilterableTable/FilterableTableStyles.less index 00786db70c9f3..d921149ae5fa5 100644 --- a/superset-frontend/src/components/FilterableTable/FilterableTableStyles.less +++ b/superset-frontend/src/components/FilterableTable/FilterableTableStyles.less @@ -57,6 +57,7 @@ .grid-header-cell { font-weight: @font-weight-bold; + cursor: pointer; } .ReactVirtualized__Table__headerColumn:last-of-type,