fix: respond to code review

- move useDataViewer
- improve keyboard accessibility
- improve localize

Signed-off-by: Scott Dover <[email protected]>

Author: Scott Dover

client/src/webview/ColumnHeader.tsx

@@ -4,8 +4,6 @@ import { useRef } from "react";
 
 import { AgColumn, GridApi } from "ag-grid-community";
 
-import { ColumnMenuProps, useColumnMenu } from "./ColumnMenu";
-
 const getIconForColumnType = (type: string) => {
   switch (type.toLocaleLowerCase()) {
     case "float":
@@ -31,15 +29,15 @@ const ColumnHeader = ({
   column,
   currentColumn: getCurrentColumn,
   columnType,
-  setColumnMenu,
   theme,
+  displayMenuForColumn,
 }: {
   api: GridApi;
   column: AgColumn;
   currentColumn: () => AgColumn | undefined;
   columnType: string;
-  setColumnMenu: (props: ColumnMenuProps) => void;
   theme: string;
+  displayMenuForColumn: (api: GridApi, column: AgColumn, rect: DOMRect) => void;
 }) => {
   const ref = useRef<HTMLButtonElement>(undefined!);
   const currentColumn = getCurrentColumn();
@@ -51,17 +49,8 @@ const ColumnHeader = ({
   const dropdownClassname =
     currentColumn?.colId === column.colId ? "active dropdown" : "dropdown";
 
-  const getColumnMenu = useColumnMenu({ api, theme });
-  const displayColumnMenu = () => {
-    if (currentColumn) {
-      return setColumnMenu(undefined);
-    }
-    setColumnMenu(
-      getColumnMenu(column, ref.current.getBoundingClientRect(), () =>
-        setColumnMenu(undefined),
-      ),
-    );
-  };
+  const displayColumnMenu = () =>
+    displayMenuForColumn(api, column, ref.current.getBoundingClientRect());
 
   return (
     <div className={`ag-cell-label-container ${theme}`} role="presentation">
@@ -77,7 +66,12 @@ const ColumnHeader = ({
           )}
         </span>
         <div className={dropdownClassname}>
-          <button ref={ref} type="button" onClick={displayColumnMenu}>
+          <button
+            ref={ref}
+            type="button"
+            onClick={displayColumnMenu}
+            tabIndex={-1}
+          >
             <span />
           </button>
         </div>

client/src/webview/ColumnMenu.tsx

@@ -17,64 +17,58 @@ export interface ColumnMenuProps {
   top: number;
 }
 
-export const useColumnMenu = ({
-  api,
-  theme,
-}: {
-  api: GridApi;
-  theme: ColumnMenuProps["theme"];
-}) => {
-  const applyColumnState = (state: ColumnState[]) => {
-    api.applyColumnState({ state, defaultState: { sort: null } });
-    api.ensureIndexVisible(0);
-  };
-
-  return (
-    column: AgColumn,
-    { height, top, left }: DOMRect,
-    dismissMenu: () => void,
-  ) => ({
-    column,
-    dismissMenu,
-    hasSort: api.getColumnState().some((c) => c.sort),
-    left,
-    theme,
-    top: top + height,
-    sortColumn: (direction: "asc" | "desc" | null) => {
-      const newColumnState = api.getColumnState().filter((c) => c.sort);
-      const colIndex = newColumnState.findIndex(
-        (c) => c.colId === column.colId,
-      );
-      if (colIndex === -1) {
-        newColumnState.push({
-          colId: column.colId,
-          sort: direction,
-          sortIndex: newColumnState.length,
-        });
-      } else {
-        newColumnState[colIndex].sort = direction;
-      }
-      applyColumnState(newColumnState);
-    },
-    removeAllSorting: () =>
-      applyColumnState(
-        api
-          .getColumnState()
-          .filter((c) => c.sort)
-          .map((c) => ({ colId: c.colId, sort: null })),
-      ),
-    removeFromSort: () =>
-      applyColumnState(
-        api
-          .getColumnState()
-          .sort((a, b) => a.sortIndex - b.sortIndex)
-          .filter((c) => c.sort && c.colId !== column.colId)
-          // After we remove the column, lets reindex what's left
-          .map((c, sortIndex) => ({ ...c, sortIndex })),
-      ),
-  });
+const applyColumnState = (api: GridApi, state: ColumnState[]) => {
+  api.applyColumnState({ state, defaultState: { sort: null } });
+  api.ensureIndexVisible(0);
 };
 
+export const getColumnMenu = (
+  api: GridApi,
+  theme: ColumnMenuProps["theme"],
+  column: AgColumn,
+  { height, top, left }: DOMRect,
+  dismissMenu: () => void,
+) => ({
+  column,
+  dismissMenu,
+  hasSort: api.getColumnState().some((c) => c.sort),
+  left,
+  theme,
+  top: top + height,
+  sortColumn: (direction: "asc" | "desc" | null) => {
+    const newColumnState = api.getColumnState().filter((c) => c.sort);
+    const colIndex = newColumnState.findIndex((c) => c.colId === column.colId);
+    if (colIndex === -1) {
+      newColumnState.push({
+        colId: column.colId,
+        sort: direction,
+        sortIndex: newColumnState.length,
+      });
+    } else {
+      newColumnState[colIndex].sort = direction;
+    }
+    applyColumnState(api, newColumnState);
+  },
+  removeAllSorting: () =>
+    applyColumnState(
+      api,
+      api
+        .getColumnState()
+        .filter((c) => c.sort)
+        .map((c) => ({ colId: c.colId, sort: null })),
+    ),
+  removeFromSort: () =>
+    applyColumnState(
+      api,
+      api
+        .getColumnState()
+        .sort((a, b) => a.sortIndex - b.sortIndex)
+        .filter((c) => c.sort && c.colId !== column.colId)
+        // After we remove the column, lets reindex what's left
+        .map((c, sortIndex) => ({ ...c, sortIndex })),
+    ),
+});
+
 const ColumnMenu = ({
   column,
   dismissMenu,

client/src/webview/GridMenu.tsx

@@ -105,7 +105,6 @@ const GridMenu = ({
                   className={`ag-menu-option ${index === activeIndex ? "ag-menu-option-active" : ""} ${menuItem.disabled ? "ag-menu-option-disabled" : ""}`}
                   role="menuitem"
                   aria-haspopup="menu"
-                  tabIndex={-1}
                   key={menuItem.name}
                   onMouseEnter={() => {
                     if (menuItem.disabled) {

client/src/webview/localize.ts

@@ -5,7 +5,7 @@
 // This expects for translations to be defined in a script block with an id of
 // `l10-messages`.
 let localizedTerms = null;
-const localize = (term: string) => {
+const localize = (term: string, replacments?: Record<string, string>) => {
   if (!localizedTerms) {
     try {
       localizedTerms = JSON.parse(
@@ -17,7 +17,15 @@ const localize = (term: string) => {
     }
   }
 
-  return localizedTerms[term] ?? term;
+  let translatedString = localizedTerms[term] ?? term;
+  if (replacments) {
+    Object.entries(replacments).forEach(
+      ([key, value]) =>
+        (translatedString = translatedString.replace(`{${key}}`, value)),
+    );
+  }
+
+  return translatedString;
 };
 
 export default localize;

client/src/webview/useDataViewer.tsx renamed to client/src/webview/useDataViewer.ts

@@ -3,19 +3,22 @@
 import { useCallback, useEffect, useRef, useState } from "react";
 
 import {
+  AgColumn,
   AllCommunityModule,
   ColDef,
+  GridApi,
   GridReadyEvent,
   IGetRowsParams,
   ModuleRegistry,
   SortModelItem,
+  SuppressHeaderKeyboardEventParams,
 } from "ag-grid-community";
 import { v4 } from "uuid";
 
 import { TableData } from "../components/LibraryNavigator/types";
 import { Column } from "../connection/rest/api/compute";
 import ColumnHeader from "./ColumnHeader";
-import { ColumnMenuProps } from "./ColumnMenu";
+import { ColumnMenuProps, getColumnMenu } from "./ColumnMenu";
 
 declare const acquireVsCodeApi;
 const vscode = acquireVsCodeApi();
@@ -142,6 +145,18 @@ const useDataViewer = (theme: string) => {
     [columns],
   );
 
+  const displayMenuForColumn = useCallback(
+    (api: GridApi, column: AgColumn, rect: DOMRect) => {
+      if (columnMenuRef.current?.column) {
+        return setColumnMenu(undefined);
+      }
+      setColumnMenu(
+        getColumnMenu(api, theme, column, rect, () => setColumnMenu(undefined)),
+      );
+    },
+    [theme],
+  );
+
   useEffect(() => {
     if (columns.length > 0) {
       return;
@@ -153,11 +168,42 @@ const useDataViewer = (theme: string) => {
         headerComponent: ColumnHeader,
         headerComponentParams: {
           columnType: column.type,
-          setColumnMenu,
           currentColumn: () => columnMenuRef.current?.column,
+          displayMenuForColumn,
           theme,
         },
+        suppressHeaderKeyboardEvent: (
+          params: SuppressHeaderKeyboardEventParams,
+        ) => {
+          // If a user tabs to a different column, dismiss the column menu
+          if (params.event.key === "Tab") {
+            setColumnMenu(undefined);
+            return false;
+          }
+          if (
+            params.event.key === "Enter" ||
+            (params.event.key === "F10" && params.event.shiftKey)
+          ) {
+            const dropdownButton =
+              // eslint-disable-next-line @typescript-eslint/consistent-type-assertions
+              (params.event.target as HTMLElement).querySelector(
+                ".dropdown > button",
+              );
+            if (!dropdownButton) {
+              return true;
+            }
+            displayMenuForColumn(
+              params.api,
+              // eslint-disable-next-line @typescript-eslint/consistent-type-assertions
+              params.column as AgColumn,
+              dropdownButton.getBoundingClientRect(),
+            );
+            return true;
+          }
+          return false;
+        },
       }));
+
       columns.unshift({
         field: "#",
         suppressMovable: true,
@@ -166,7 +212,7 @@ const useDataViewer = (theme: string) => {
 
       setColumns(columns);
     });
-  }, [columns.length, theme]);
+  }, [columns.length, theme, displayMenuForColumn]);
 
   useEffect(() => {
     window.addEventListener("contextmenu", contextMenuHandler, true);