Initially order null columns last in dashboard activity tables (#6381)

* Put null values at the bottom in dashboard activity tables * Every dashboard activity table now decides initial order direction per column
hypothesis · Jun 20, 2024 · fa7a24f · fa7a24f
1 parent c445127
commit fa7a24f
Show file tree

Hide file tree

Showing 7 changed files with 121 additions and 70 deletions.
diff --git a/lms/static/scripts/frontend_apps/components/dashboard/AssignmentActivity.tsx b/lms/static/scripts/frontend_apps/components/dashboard/AssignmentActivity.tsx
@@ -72,12 +72,27 @@ export default function AssignmentActivity() {
           students.error ? 'Could not load students' : 'No students found'
         }
         rows={rows}
-        columnNames={{
-          display_name: 'Student',
-          annotations: 'Annotations',
-          replies: 'Replies',
-          last_activity: 'Last Activity',
-        }}
+        columns={[
+          {
+            field: 'display_name',
+            label: 'Student',
+          },
+          {
+            field: 'annotations',
+            label: 'Annotations',
+            initialOrderDirection: 'descending',
+          },
+          {
+            field: 'replies',
+            label: 'Replies',
+            initialOrderDirection: 'descending',
+          },
+          {
+            field: 'last_activity',
+            label: 'Last Activity',
+            initialOrderDirection: 'descending',
+          },
+        ]}
         defaultOrderField="display_name"
         renderItem={(stats, field) => {
           if (['annotations', 'replies'].includes(field)) {

diff --git a/lms/static/scripts/frontend_apps/components/dashboard/CourseActivity.tsx b/lms/static/scripts/frontend_apps/components/dashboard/CourseActivity.tsx
@@ -69,12 +69,27 @@ export default function CourseActivity() {
             : 'No assignments found'
         }
         rows={rows}
-        columnNames={{
-          title: 'Assignment',
-          annotations: 'Annotations',
-          replies: 'Replies',
-          last_activity: 'Last Activity',
-        }}
+        columns={[
+          {
+            field: 'title',
+            label: 'Assignment',
+          },
+          {
+            field: 'annotations',
+            label: 'Annotations',
+            initialOrderDirection: 'descending',
+          },
+          {
+            field: 'replies',
+            label: 'Replies',
+            initialOrderDirection: 'descending',
+          },
+          {
+            field: 'last_activity',
+            label: 'Last Activity',
+            initialOrderDirection: 'descending',
+          },
+        ]}
         defaultOrderField="title"
         renderItem={(stats, field) => {
           if (['annotations', 'replies'].includes(field)) {

diff --git a/lms/static/scripts/frontend_apps/components/dashboard/OrderableActivityTable.tsx b/lms/static/scripts/frontend_apps/components/dashboard/OrderableActivityTable.tsx
@@ -5,11 +5,17 @@ import type { OrderDirection } from '@hypothesis/frontend-shared/lib/types';
 import { useMemo, useState } from 'preact/hooks';
 import { useLocation } from 'wouter-preact';
 
+export type OrderableActivityTableColumn<T> = {
+  field: keyof T;
+  label: string;
+  initialOrderDirection?: OrderDirection;
+};
+
 export type OrderableActivityTableProps<T> = Pick<
   DataTableProps<T>,
   'emptyMessage' | 'rows' | 'renderItem' | 'loading' | 'title'
 > & {
-  columnNames: Partial<Record<keyof T, string>>;
+  columns: OrderableActivityTableColumn<T>[];
   defaultOrderField: keyof T;
 
   /**
@@ -19,33 +25,14 @@ export type OrderableActivityTableProps<T> = Pick<
   navigateOnConfirmRow?: (row: T) => string;
 };
 
-/**
- * List of columns which should start sorted in descending order.
- *
- * This is because for numeric columns ("annotations" and "replies") users will
- * usually want to see the higher values first.
- * Similarly, for date columns ("last_activity") users will want to see most
- * recent values first.
- */
-const descendingOrderColumns: readonly string[] = [
-  // Annotation metrics
-  'last_activity',
-  'annotations',
-  'replies',
-
-  // Course metrics
-  'last_launched',
-  'assignments',
-];
-
 /**
  * Annotation activity table for dashboard views. Includes built-in support for
  * sorting columns.
  */
 export default function OrderableActivityTable<T>({
   defaultOrderField,
   rows,
-  columnNames,
+  columns,
   navigateOnConfirmRow,
   ...restOfTableProps
 }: OrderableActivityTableProps<T>) {
@@ -54,41 +41,45 @@ export default function OrderableActivityTable<T>({
     direction: 'ascending',
   });
   const orderedRows = useOrderedRows(rows, order);
-  const columns = useMemo(
+  const dataTableColumns = useMemo(
     () =>
-      Object.entries(columnNames).map(([field, label], index) => ({
-        field: field as keyof T,
-        label: label as string,
+      columns.map(({ field, label }, index) => ({
+        field,
+        label,
         classes: index === 0 ? 'w-[60%]' : undefined,
       })),
-    [columnNames],
+    [columns],
   );
   // Map of column name to initial sort order
   const orderableColumns = useMemo(
     () =>
-      (Object.keys(columnNames) as Array<keyof T>).reduce<
-        Partial<Record<keyof T, OrderDirection>>
-      >((acc, columnName) => {
-        acc[columnName] =
-          typeof columnName === 'string' &&
-          descendingOrderColumns.includes(columnName)
-            ? 'descending'
-            : 'ascending';
-        return acc;
-      }, {}),
-    [columnNames],
+      columns.reduce<Partial<Record<keyof T, OrderDirection>>>(
+        (acc, { field, initialOrderDirection = 'ascending' }) => {
+          acc[field] = initialOrderDirection;
+          return acc;
+        },
+        {},
+      ),
+    [columns],
   );
   const [, navigate] = useLocation();
 
   return (
     <DataTable
       grid
       striped={false}
-      columns={columns}
+      columns={dataTableColumns}
       rows={orderedRows}
       orderableColumns={orderableColumns}
       order={order}
-      onOrderChange={setOrder}
+      onOrderChange={order =>
+        setOrder({
+          ...order,
+          // Every column should start with nulls last, and move them first
+          // when order direction changes
+          nullsLast: order.direction === orderableColumns[order.field],
+        })
+      }
       onConfirmRow={
         navigateOnConfirmRow
           ? row => navigate(navigateOnConfirmRow(row))

diff --git a/lms/static/scripts/frontend_apps/components/dashboard/OrganizationActivity.tsx b/lms/static/scripts/frontend_apps/components/dashboard/OrganizationActivity.tsx
@@ -55,11 +55,22 @@ export default function OrganizationActivity({
           courses.error ? 'Could not load courses' : 'No courses found'
         }
         rows={rows}
-        columnNames={{
-          title: 'Course Title',
-          assignments: 'Assignments',
-          last_launched: 'Last launched',
-        }}
+        columns={[
+          {
+            field: 'title',
+            label: 'Course title',
+          },
+          {
+            field: 'assignments',
+            label: 'Assignments',
+            initialOrderDirection: 'descending',
+          },
+          {
+            field: 'last_launched',
+            label: 'Last launched',
+            initialOrderDirection: 'descending',
+          },
+        ]}
         defaultOrderField="title"
         renderItem={(stats, field) => {
           if (field === 'assignments') {

diff --git a/lms/static/scripts/frontend_apps/components/dashboard/test/OrderableActivityTable-test.js b/lms/static/scripts/frontend_apps/components/dashboard/test/OrderableActivityTable-test.js
@@ -1,4 +1,5 @@
 import { mount } from 'enzyme';
+import sinon from 'sinon';
 
 import OrderableActivityTable, { $imports } from '../OrderableActivityTable';
 
@@ -43,12 +44,24 @@ describe('OrderableActivityTable', () => {
     return mount(
       <OrderableActivityTable
         rows={rows}
-        columnNames={{
-          display_name: 'Name',
-          last_activity: 'Last activity',
-          annotations: 'Annotations',
-          replies: 'Replies',
-        }}
+        columns={[
+          {
+            field: 'display_name',
+            label: 'Name',
+          },
+          {
+            field: 'last_activity',
+            label: 'Last activity',
+          },
+          {
+            field: 'annotations',
+            label: 'Annotations',
+          },
+          {
+            field: 'replies',
+            label: 'Replies',
+          },
+        ]}
         defaultOrderField="display_name"
         navigateOnConfirmRow={navigateOnConfirmRow}
       />,
@@ -58,6 +71,7 @@ describe('OrderableActivityTable', () => {
   [
     {
       orderToSet: { field: 'annotations', direction: 'descending' },
+      expectedNullsLast: false,
       expectedStudents: [
         {
           display_name: 'b',
@@ -81,6 +95,7 @@ describe('OrderableActivityTable', () => {
     },
     {
       orderToSet: { field: 'replies', direction: 'ascending' },
+      expectedNullsLast: true,
       expectedStudents: [
         {
           display_name: 'b',
@@ -104,6 +119,7 @@ describe('OrderableActivityTable', () => {
     },
     {
       orderToSet: { field: 'last_activity', direction: 'descending' },
+      expectedNullsLast: false,
       expectedStudents: [
         {
           display_name: 'a',
@@ -125,7 +141,7 @@ describe('OrderableActivityTable', () => {
         },
       ],
     },
-  ].forEach(({ orderToSet, expectedStudents }) => {
+  ].forEach(({ orderToSet, expectedNullsLast, expectedStudents }) => {
     it('reorders students on order change', () => {
       const wrapper = createComponent();
       const getRows = () => wrapper.find('DataTable').prop('rows');
@@ -162,7 +178,10 @@ describe('OrderableActivityTable', () => {
       ]);
 
       setOrder(orderToSet);
-      assert.deepEqual(getOrder(), orderToSet);
+      assert.deepEqual(getOrder(), {
+        ...orderToSet,
+        nullsLast: expectedNullsLast,
+      });
       assert.deepEqual(getRows(), expectedStudents);
     });
   });

diff --git a/package.json b/package.json
@@ -26,7 +26,7 @@
     "@babel/preset-react": "^7.24.6",
     "@babel/preset-typescript": "^7.24.6",
     "@hypothesis/frontend-build": "^3.0.0",
-    "@hypothesis/frontend-shared": "^7.10.1",
+    "@hypothesis/frontend-shared": "^7.11.0",
     "@rollup/plugin-babel": "^6.0.4",
     "@rollup/plugin-commonjs": "^25.0.8",
     "@rollup/plugin-node-resolve": "^15.2.3",

diff --git a/yarn.lock b/yarn.lock
@@ -1988,15 +1988,15 @@ __metadata:
   languageName: node
   linkType: hard
 
-"@hypothesis/frontend-shared@npm:^7.10.1":
-  version: 7.10.1
-  resolution: "@hypothesis/frontend-shared@npm:7.10.1"
+"@hypothesis/frontend-shared@npm:^7.11.0":
+  version: 7.11.0
+  resolution: "@hypothesis/frontend-shared@npm:7.11.0"
   dependencies:
     highlight.js: ^11.6.0
     wouter-preact: ^3.0.0
   peerDependencies:
     preact: ^10.4.0
-  checksum: 758c415d8e68325a3c630122e76bd40895cafbfa00404829549380c873aed6e8fd937c27da5a0632240d547c262425b1374b75e951d498d546aa19c6894daff1
+  checksum: 12d10503b2a51a8c5d690f0f892341698465f49d3c760574a838918879a7a0151ddf0994f422515264224f6462a5b1baf1169f4f16ca6f5f9b8d7b4d8065cc4b
   languageName: node
   linkType: hard
 
@@ -7193,7 +7193,7 @@ __metadata:
     "@babel/preset-react": ^7.24.6
     "@babel/preset-typescript": ^7.24.6
     "@hypothesis/frontend-build": ^3.0.0
-    "@hypothesis/frontend-shared": ^7.10.1
+    "@hypothesis/frontend-shared": ^7.11.0
     "@hypothesis/frontend-testing": ^1.2.2
     "@rollup/plugin-babel": ^6.0.4
     "@rollup/plugin-commonjs": ^25.0.8