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 <corbin@Corbins-MacBook-Pro.local>

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');
   });
 });

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)}
         >
-          <div>{label}</div>
+          {label}
+          {this.state.sortBy === label && (
+            <SortIndicator sortDirection={this.state.sortDirection} />
+          )}
         </div>
       </Tooltip>
     );
@@ -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 ? <i className="text-muted">{cellText}</i> : 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;

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,