From ee555b6676096b299bc96d49ce7ceda759ddf776 Mon Sep 17 00:00:00 2001
From: Josh Black <josh@josh.black>
Date: Tue, 13 Nov 2018 09:28:48 -0600
Subject: [PATCH] fix(DataTable): update sort order to go ASC -> DESC -> NONE
 (#1535)

* fix(DataTable): update sort order to go ASC -> DESC -> NONE

* fix(data-table): update Rort -> Sort

Co-Authored-By: joshblack <josh@josh.black>

* fix(data-table): update Rort -> Sort

Co-Authored-By: joshblack <josh@josh.black>

* feat(data-table): add aria-sort property to TableHeader
---
 src/components/DataTable/TableHeader.js       | 26 ++++++---
 .../DataTable/__tests__/DataTable-test.js     |  6 +--
 .../__snapshots__/DataTable-test.js.snap      | 54 ++++++++++---------
 .../getDerivedStateFromProps-test.js          |  2 +-
 .../DataTable/state/__tests__/sorting-test.js | 28 +++++-----
 src/components/DataTable/state/sorting.js     | 10 ++--
 .../DataTable/tools/__tests__/sorting-test.js |  8 +--
 src/components/DataTable/tools/sorting.js     |  4 +-
 8 files changed, 79 insertions(+), 59 deletions(-)

diff --git a/src/components/DataTable/TableHeader.js b/src/components/DataTable/TableHeader.js
index 1511239503c0..36a33ec976c7 100644
--- a/src/components/DataTable/TableHeader.js
+++ b/src/components/DataTable/TableHeader.js
@@ -12,16 +12,29 @@ const translationKeys = {
 const translateWithId = (key, { sortDirection, isSortHeader, sortStates }) => {
   if (key === translationKeys.iconDescription) {
     if (isSortHeader) {
-      const order =
-        sortDirection === sortStates.DESC ? 'descending' : 'ascending';
-      return `Sort rows by this header in ${order} order`;
+      // When transitioning, we know that the sequence of states is as follows:
+      // NONE -> ASC -> DESC -> NONE
+      if (sortDirection === sortStates.NONE) {
+        return 'Sort rows by this header in ascending order';
+      }
+      if (sortDirection === sortStates.ASC) {
+        return 'Sort rows by this header in descending order';
+      }
+
+      return 'Unsort rows by this header';
     }
-    return `Sort rows by this header in descending order`;
+    return 'Sort rows by this header in ascending order';
   }
 
   return '';
 };
 
+const sortDirections = {
+  [sortStates.NONE]: 'none',
+  [sortStates.ASC]: 'ascending',
+  [sortStates.DESC]: 'descending',
+};
+
 const TableHeader = ({
   className: headerClassName,
   children,
@@ -46,11 +59,12 @@ const TableHeader = ({
     'bx--table-sort-v2--active':
       isSortHeader && sortDirection !== sortStates.NONE,
     'bx--table-sort-v2--ascending':
-      isSortHeader && sortDirection === sortStates.ASC,
+      isSortHeader && sortDirection === sortStates.DESC,
   });
+  const ariaSort = !isSortHeader ? 'none' : sortDirections[sortDirection];
 
   return (
-    <th scope={scope} className={headerClassName}>
+    <th scope={scope} className={headerClassName} aria-sort={ariaSort}>
       <button className={className} onClick={onClick} {...rest}>
         <span className="bx--table-header-label">{children}</span>
         <Icon
diff --git a/src/components/DataTable/__tests__/DataTable-test.js b/src/components/DataTable/__tests__/DataTable-test.js
index d790716a98a0..27b90e7354d1 100644
--- a/src/components/DataTable/__tests__/DataTable-test.js
+++ b/src/components/DataTable/__tests__/DataTable-test.js
@@ -148,7 +148,7 @@ describe('DataTable', () => {
       expect(wrapper.state('rowIds')).toEqual(['a', 'b', 'c']);
     });
 
-    it('should reset to DESC ordering when another header is clicked', () => {
+    it('should reset to ASC ordering when another header is clicked', () => {
       const wrapper = mount(<DataTable {...mockProps} />);
       const firstHeader = getHeaderAt(wrapper, 0);
       const secondHeader = getHeaderAt(wrapper, 1);
@@ -158,10 +158,10 @@ describe('DataTable', () => {
 
       firstHeader.simulate('click');
       expect(wrapper.state('rowIds')).toEqual(['c', 'b', 'a']);
-      expect(wrapper.state('sortDirection')).toBe(sortStates.ASC);
+      expect(wrapper.state('sortDirection')).toBe(sortStates.DESC);
 
       secondHeader.simulate('click');
-      expect(wrapper.state('sortDirection')).toBe(sortStates.DESC);
+      expect(wrapper.state('sortDirection')).toBe(sortStates.ASC);
     });
   });
 
diff --git a/src/components/DataTable/__tests__/__snapshots__/DataTable-test.js.snap b/src/components/DataTable/__tests__/__snapshots__/DataTable-test.js.snap
index bb450e337fee..95fd3794af1d 100644
--- a/src/components/DataTable/__tests__/__snapshots__/DataTable-test.js.snap
+++ b/src/components/DataTable/__tests__/__snapshots__/DataTable-test.js.snap
@@ -167,6 +167,7 @@ exports[`DataTable selection should have select-all default to un-checked if no
                     translateWithId={[Function]}
                   >
                     <th
+                      aria-sort="none"
                       scope="col"
                     >
                       <button
@@ -180,7 +181,7 @@ exports[`DataTable selection should have select-all default to un-checked if no
                         </span>
                         <Icon
                           className="bx--table-sort-v2__icon"
-                          description="Sort rows by this header in descending order"
+                          description="Sort rows by this header in ascending order"
                           fillRule="evenodd"
                           icon={
                             Object {
@@ -208,8 +209,8 @@ exports[`DataTable selection should have select-all default to un-checked if no
                           role="img"
                         >
                           <svg
-                            alt="Sort rows by this header in descending order"
-                            aria-label="Sort rows by this header in descending order"
+                            alt="Sort rows by this header in ascending order"
+                            aria-label="Sort rows by this header in ascending order"
                             className="bx--table-sort-v2__icon"
                             fillRule="evenodd"
                             height="5"
@@ -218,7 +219,7 @@ exports[`DataTable selection should have select-all default to un-checked if no
                             width="10"
                           >
                             <title>
-                              Sort rows by this header in descending order
+                              Sort rows by this header in ascending order
                             </title>
                             <path
                               d="M0 5L5 .002 10 5z"
@@ -239,6 +240,7 @@ exports[`DataTable selection should have select-all default to un-checked if no
                     translateWithId={[Function]}
                   >
                     <th
+                      aria-sort="none"
                       scope="col"
                     >
                       <button
@@ -252,7 +254,7 @@ exports[`DataTable selection should have select-all default to un-checked if no
                         </span>
                         <Icon
                           className="bx--table-sort-v2__icon"
-                          description="Sort rows by this header in descending order"
+                          description="Sort rows by this header in ascending order"
                           fillRule="evenodd"
                           icon={
                             Object {
@@ -280,8 +282,8 @@ exports[`DataTable selection should have select-all default to un-checked if no
                           role="img"
                         >
                           <svg
-                            alt="Sort rows by this header in descending order"
-                            aria-label="Sort rows by this header in descending order"
+                            alt="Sort rows by this header in ascending order"
+                            aria-label="Sort rows by this header in ascending order"
                             className="bx--table-sort-v2__icon"
                             fillRule="evenodd"
                             height="5"
@@ -290,7 +292,7 @@ exports[`DataTable selection should have select-all default to un-checked if no
                             width="10"
                           >
                             <title>
-                              Sort rows by this header in descending order
+                              Sort rows by this header in ascending order
                             </title>
                             <path
                               d="M0 5L5 .002 10 5z"
@@ -634,6 +636,7 @@ exports[`DataTable selection should render 1`] = `
                     translateWithId={[Function]}
                   >
                     <th
+                      aria-sort="none"
                       scope="col"
                     >
                       <button
@@ -647,7 +650,7 @@ exports[`DataTable selection should render 1`] = `
                         </span>
                         <Icon
                           className="bx--table-sort-v2__icon"
-                          description="Sort rows by this header in descending order"
+                          description="Sort rows by this header in ascending order"
                           fillRule="evenodd"
                           icon={
                             Object {
@@ -675,8 +678,8 @@ exports[`DataTable selection should render 1`] = `
                           role="img"
                         >
                           <svg
-                            alt="Sort rows by this header in descending order"
-                            aria-label="Sort rows by this header in descending order"
+                            alt="Sort rows by this header in ascending order"
+                            aria-label="Sort rows by this header in ascending order"
                             className="bx--table-sort-v2__icon"
                             fillRule="evenodd"
                             height="5"
@@ -685,7 +688,7 @@ exports[`DataTable selection should render 1`] = `
                             width="10"
                           >
                             <title>
-                              Sort rows by this header in descending order
+                              Sort rows by this header in ascending order
                             </title>
                             <path
                               d="M0 5L5 .002 10 5z"
@@ -706,6 +709,7 @@ exports[`DataTable selection should render 1`] = `
                     translateWithId={[Function]}
                   >
                     <th
+                      aria-sort="none"
                       scope="col"
                     >
                       <button
@@ -719,7 +723,7 @@ exports[`DataTable selection should render 1`] = `
                         </span>
                         <Icon
                           className="bx--table-sort-v2__icon"
-                          description="Sort rows by this header in descending order"
+                          description="Sort rows by this header in ascending order"
                           fillRule="evenodd"
                           icon={
                             Object {
@@ -747,8 +751,8 @@ exports[`DataTable selection should render 1`] = `
                           role="img"
                         >
                           <svg
-                            alt="Sort rows by this header in descending order"
-                            aria-label="Sort rows by this header in descending order"
+                            alt="Sort rows by this header in ascending order"
+                            aria-label="Sort rows by this header in ascending order"
                             className="bx--table-sort-v2__icon"
                             fillRule="evenodd"
                             height="5"
@@ -757,7 +761,7 @@ exports[`DataTable selection should render 1`] = `
                             width="10"
                           >
                             <title>
-                              Sort rows by this header in descending order
+                              Sort rows by this header in ascending order
                             </title>
                             <path
                               d="M0 5L5 .002 10 5z"
@@ -1723,6 +1727,7 @@ exports[`DataTable should render 1`] = `
                     translateWithId={[Function]}
                   >
                     <th
+                      aria-sort="none"
                       scope="col"
                     >
                       <button
@@ -1736,7 +1741,7 @@ exports[`DataTable should render 1`] = `
                         </span>
                         <Icon
                           className="bx--table-sort-v2__icon"
-                          description="Sort rows by this header in descending order"
+                          description="Sort rows by this header in ascending order"
                           fillRule="evenodd"
                           icon={
                             Object {
@@ -1764,8 +1769,8 @@ exports[`DataTable should render 1`] = `
                           role="img"
                         >
                           <svg
-                            alt="Sort rows by this header in descending order"
-                            aria-label="Sort rows by this header in descending order"
+                            alt="Sort rows by this header in ascending order"
+                            aria-label="Sort rows by this header in ascending order"
                             className="bx--table-sort-v2__icon"
                             fillRule="evenodd"
                             height="5"
@@ -1774,7 +1779,7 @@ exports[`DataTable should render 1`] = `
                             width="10"
                           >
                             <title>
-                              Sort rows by this header in descending order
+                              Sort rows by this header in ascending order
                             </title>
                             <path
                               d="M0 5L5 .002 10 5z"
@@ -1795,6 +1800,7 @@ exports[`DataTable should render 1`] = `
                     translateWithId={[Function]}
                   >
                     <th
+                      aria-sort="none"
                       scope="col"
                     >
                       <button
@@ -1808,7 +1814,7 @@ exports[`DataTable should render 1`] = `
                         </span>
                         <Icon
                           className="bx--table-sort-v2__icon"
-                          description="Sort rows by this header in descending order"
+                          description="Sort rows by this header in ascending order"
                           fillRule="evenodd"
                           icon={
                             Object {
@@ -1836,8 +1842,8 @@ exports[`DataTable should render 1`] = `
                           role="img"
                         >
                           <svg
-                            alt="Sort rows by this header in descending order"
-                            aria-label="Sort rows by this header in descending order"
+                            alt="Sort rows by this header in ascending order"
+                            aria-label="Sort rows by this header in ascending order"
                             className="bx--table-sort-v2__icon"
                             fillRule="evenodd"
                             height="5"
@@ -1846,7 +1852,7 @@ exports[`DataTable should render 1`] = `
                             width="10"
                           >
                             <title>
-                              Sort rows by this header in descending order
+                              Sort rows by this header in ascending order
                             </title>
                             <path
                               d="M0 5L5 .002 10 5z"
diff --git a/src/components/DataTable/state/__tests__/getDerivedStateFromProps-test.js b/src/components/DataTable/state/__tests__/getDerivedStateFromProps-test.js
index 182bcf1886ef..f537cb50ef3e 100644
--- a/src/components/DataTable/state/__tests__/getDerivedStateFromProps-test.js
+++ b/src/components/DataTable/state/__tests__/getDerivedStateFromProps-test.js
@@ -61,7 +61,7 @@ describe('getDerivedStateFromProps', () => {
       expect(initialState.rowIds).toEqual(['1', '2', '0']);
       const prevState = {
         sortHeaderKey: 'sortField',
-        sortDirection: 'DESC',
+        sortDirection: 'ASC',
       };
       const nextState = getDerivedStateFromProps(mockProps, prevState);
       expect(nextState.rowIds).toEqual(['0', '1', '2']);
diff --git a/src/components/DataTable/state/__tests__/sorting-test.js b/src/components/DataTable/state/__tests__/sorting-test.js
index 782c73cdb141..ed8dc3a1597a 100644
--- a/src/components/DataTable/state/__tests__/sorting-test.js
+++ b/src/components/DataTable/state/__tests__/sorting-test.js
@@ -40,13 +40,13 @@ describe('sorting state', () => {
       mockPrevState = sortStates.NONE;
     });
 
-    it('should default to DESC', () => {
+    it('should default to ASC', () => {
       expect(
         getNextSortDirection(mockHeaderA, mockHeaderA, mockPrevState)
-      ).toBe(sortStates.DESC);
+      ).toBe(sortStates.ASC);
     });
 
-    it('should transition from DESC -> ASC -> NONE', () => {
+    it('should transition from ASC -> DESC -> NONE', () => {
       const nextState1 = getNextSortDirection(
         mockHeaderA,
         mockHeaderA,
@@ -67,13 +67,13 @@ describe('sorting state', () => {
         mockHeaderA,
         nextState3
       );
-      expect(nextState1).toBe(sortStates.DESC);
-      expect(nextState2).toBe(sortStates.ASC);
+      expect(nextState1).toBe(sortStates.ASC);
+      expect(nextState2).toBe(sortStates.DESC);
       expect(nextState3).toBe(sortStates.NONE);
-      expect(nextState4).toBe(sortStates.DESC);
+      expect(nextState4).toBe(sortStates.ASC);
     });
 
-    it('should reset to DESC if the header changes', () => {
+    it('should reset to ASC if the header changes', () => {
       const nextState1 = getNextSortDirection(
         mockHeaderA,
         mockHeaderA,
@@ -89,9 +89,9 @@ describe('sorting state', () => {
         mockHeaderB,
         nextState2
       );
-      expect(nextState1).toBe(sortStates.DESC);
-      expect(nextState2).toBe(sortStates.ASC);
-      expect(nextState3).toBe(sortStates.DESC);
+      expect(nextState1).toBe(sortStates.ASC);
+      expect(nextState2).toBe(sortStates.DESC);
+      expect(nextState3).toBe(sortStates.ASC);
     });
   });
 
@@ -130,13 +130,13 @@ describe('sorting state', () => {
       };
     });
 
-    it('should initialize in DESC order for the first header called', () => {
+    it('should initialize in ASC order for the first header called', () => {
       const sortHeaderKey = 'a';
       expect(
         getNextSortState(mockProps, mockState, { key: sortHeaderKey })
       ).toEqual({
         sortHeaderKey,
-        sortDirection: sortStates.DESC,
+        sortDirection: sortStates.ASC,
         rowIds: ['b', 'a', 'c'],
       });
     });
@@ -168,12 +168,12 @@ describe('sorting state', () => {
       );
       expect(nextState1).toEqual({
         sortHeaderKey,
-        sortDirection: sortStates.DESC,
+        sortDirection: sortStates.ASC,
         rowIds: ['b', 'a', 'c'],
       });
       expect(nextState2).toEqual({
         sortHeaderKey,
-        sortDirection: sortStates.ASC,
+        sortDirection: sortStates.DESC,
         rowIds: ['b', 'a', 'c'],
       });
       expect(nextState3).toEqual({
diff --git a/src/components/DataTable/state/sorting.js b/src/components/DataTable/state/sorting.js
index a2c4c2a97459..6e09f5d50e8f 100644
--- a/src/components/DataTable/state/sorting.js
+++ b/src/components/DataTable/state/sorting.js
@@ -29,18 +29,18 @@ export const getNextSortDirection = (prevHeader, header, prevState) => {
   // have to derive the next sort state from the previous sort state
   if (prevHeader === header) {
     // When transitioning, we know that the sequence of states is as follows:
-    // NONE -> DESC -> ASC -> NONE
+    // NONE -> ASC -> DESC -> NONE
     if (prevState === 'NONE') {
-      return sortStates.DESC;
-    }
-    if (prevState === 'DESC') {
       return sortStates.ASC;
     }
+    if (prevState === 'ASC') {
+      return sortStates.DESC;
+    }
     return sortStates.NONE;
   }
   // Otherwise, we have selected a new header and need to start off by sorting
   // in descending order by default
-  return sortStates.DESC;
+  return sortStates.ASC;
 };
 
 export const getNextSortState = (props, state, { key }) => {
diff --git a/src/components/DataTable/tools/__tests__/sorting-test.js b/src/components/DataTable/tools/__tests__/sorting-test.js
index 8627cadad774..6c32ae3525c8 100644
--- a/src/components/DataTable/tools/__tests__/sorting-test.js
+++ b/src/components/DataTable/tools/__tests__/sorting-test.js
@@ -8,8 +8,8 @@ describe('defaultSortRow', () => {
       sortStates: sortStates,
       locale: 'en',
     };
-    expect(defaultSortRow('a', 'b', sortProps)).toBeGreaterThan(0);
-    expect(defaultSortRow('1', '2', sortProps)).toBeGreaterThan(0);
+    expect(defaultSortRow('a', 'b', sortProps)).toBeLessThan(0);
+    expect(defaultSortRow('1', '2', sortProps)).toBeLessThan(0);
   });
 
   it('should sort data in descending order', () => {
@@ -18,7 +18,7 @@ describe('defaultSortRow', () => {
       sortStates: sortStates,
       locale: 'en',
     };
-    expect(defaultSortRow('a', 'b', sortProps)).toBeLessThan(0);
-    expect(defaultSortRow('1', '2', sortProps)).toBeLessThan(0);
+    expect(defaultSortRow('a', 'b', sortProps)).toBeGreaterThan(0);
+    expect(defaultSortRow('1', '2', sortProps)).toBeGreaterThan(0);
   });
 });
diff --git a/src/components/DataTable/tools/sorting.js b/src/components/DataTable/tools/sorting.js
index d1ed029d3ae0..a6257156119a 100644
--- a/src/components/DataTable/tools/sorting.js
+++ b/src/components/DataTable/tools/sorting.js
@@ -81,8 +81,8 @@ export const defaultSortRow = (
   { sortDirection, sortStates, locale }
 ) => {
   if (sortDirection === sortStates.ASC) {
-    return compare(cellB, cellA, locale);
+    return compare(cellA, cellB, locale);
   }
 
-  return compare(cellA, cellB, locale);
+  return compare(cellB, cellA, locale);
 };