From ff47a781d107e451d8f676f19dd0a6cbf7c885d1 Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandro.celaya@hypothes.is>
Date: Mon, 2 Oct 2023 14:44:53 +0200
Subject: [PATCH] Use SelectNext for import and export user selectors

---
 package.json                                  |  2 +-
 .../ShareDialog/ExportAnnotations.tsx         | 65 ++++++++++++-------
 .../ShareDialog/ImportAnnotations.tsx         | 61 ++++++++---------
 .../ShareDialog/UserAnnotationsListItem.tsx   | 21 ++++++
 .../test/ExportAnnotations-test.js            | 62 +++++++++++++-----
 .../test/ImportAnnotations-test.js            | 55 ++++++++++------
 yarn.lock                                     | 10 +--
 7 files changed, 183 insertions(+), 93 deletions(-)
 create mode 100644 src/sidebar/components/ShareDialog/UserAnnotationsListItem.tsx

diff --git a/package.json b/package.json
index 6e7d9e3e668..dd8529aeb87 100644
--- a/package.json
+++ b/package.json
@@ -12,7 +12,7 @@
     "@babel/preset-react": "^7.0.0",
     "@babel/preset-typescript": "^7.16.7",
     "@hypothesis/frontend-build": "^2.0.0",
-    "@hypothesis/frontend-shared": "^6.5.0",
+    "@hypothesis/frontend-shared": "^6.8.1",
     "@hypothesis/frontend-testing": "^1.0.1",
     "@npmcli/arborist": "^7.0.0",
     "@octokit/rest": "^20.0.1",
diff --git a/src/sidebar/components/ShareDialog/ExportAnnotations.tsx b/src/sidebar/components/ShareDialog/ExportAnnotations.tsx
index 681d239b86c..ce1059b5694 100644
--- a/src/sidebar/components/ShareDialog/ExportAnnotations.tsx
+++ b/src/sidebar/components/ShareDialog/ExportAnnotations.tsx
@@ -3,13 +3,14 @@ import {
   CardActions,
   Link,
   Input,
-  Select,
+  SelectNext,
 } from '@hypothesis/frontend-shared';
-import { useCallback, useMemo, useState } from 'preact/hooks';
+import { useCallback, useId, useMemo, useState } from 'preact/hooks';
 
 import { downloadJSONFile } from '../../../shared/download-json-file';
 import type { APIAnnotationData } from '../../../types/api';
 import { annotationDisplayName } from '../../helpers/annotation-user';
+import type { UserAnnotations } from '../../helpers/annotations-by-user';
 import { annotationsByUser } from '../../helpers/annotations-by-user';
 import { suggestedFilename } from '../../helpers/export-annotations';
 import { withServices } from '../../service-context';
@@ -17,6 +18,7 @@ import type { AnnotationsExporter } from '../../services/annotations-exporter';
 import type { ToastMessengerService } from '../../services/toast-messenger';
 import { useSidebarStore } from '../../store';
 import LoadingSpinner from './LoadingSpinner';
+import { UserAnnotationsListItem } from './UserAnnotationsListItem';
 
 export type ExportAnnotationsProps = {
   // injected
@@ -51,9 +53,23 @@ function ExportAnnotations({
     [exportableAnnotations, getDisplayName],
   );
 
-  // User whose annotations are going to be exported. Preselect current user
+  // User whose annotations are going to be exported.
   const currentUser = store.profile().userid;
-  const [selectedUser, setSelectedUser] = useState(currentUser);
+  const allAnnotationsOption: Omit<UserAnnotations, 'userid'> = useMemo(
+    () => ({
+      annotations: exportableAnnotations,
+      displayName: 'All annotations',
+    }),
+    [exportableAnnotations],
+  );
+  const [selectedUser, setSelectedUser] = useState(
+    // Try to preselect current user
+    userList.find(userInfo => userInfo.userid === currentUser) ??
+      allAnnotationsOption,
+  );
+
+  const fileInputId = useId();
+  const userSelectId = useId();
 
   const draftCount = store.countDrafts();
 
@@ -76,8 +92,7 @@ function ExportAnnotations({
 
     try {
       const annotationsToExport =
-        userList.find(item => item.userid === selectedUser)?.annotations ??
-        exportableAnnotations;
+        selectedUser?.annotations ?? exportableAnnotations;
       const filename = `${customFilename ?? defaultFilename}.json`;
       const exportData =
         annotationsExporter.buildExportContent(annotationsToExport);
@@ -113,14 +128,14 @@ function ExportAnnotations({
           </p>
           <label
             data-testid="export-count"
-            htmlFor="export-filename"
+            htmlFor={fileInputId}
             className="font-medium"
           >
             Name of export file:
           </label>
           <Input
             data-testid="export-filename"
-            id="export-filename"
+            id={fileInputId}
             defaultValue={defaultFilename}
             value={customFilename}
             onChange={e =>
@@ -129,28 +144,30 @@ function ExportAnnotations({
             required
             maxLength={250}
           />
-          <label htmlFor="export-user" className="block font-medium">
+          <label htmlFor={userSelectId} className="block font-medium">
             Select which user{"'"}s annotations to export:
           </label>
-          <Select
-            id="export-user"
-            onChange={e =>
-              setSelectedUser((e.target as HTMLSelectElement).value || null)
+          <SelectNext
+            value={selectedUser}
+            onChange={setSelectedUser}
+            buttonId={userSelectId}
+            buttonContent={
+              <UserAnnotationsListItem userAnnotations={selectedUser} />
             }
           >
-            <option value="" selected={!selectedUser}>
-              All annotations ({exportableAnnotations.length})
-            </option>
+            <SelectNext.Option value={allAnnotationsOption}>
+              {() => (
+                <UserAnnotationsListItem
+                  userAnnotations={allAnnotationsOption}
+                />
+              )}
+            </SelectNext.Option>
             {userList.map(userInfo => (
-              <option
-                key={userInfo.userid}
-                value={userInfo.userid}
-                selected={userInfo.userid === selectedUser}
-              >
-                {userInfo.displayName} ({userInfo.annotations.length})
-              </option>
+              <SelectNext.Option key={userInfo.userid} value={userInfo}>
+                {() => <UserAnnotationsListItem userAnnotations={userInfo} />}
+              </SelectNext.Option>
             ))}
-          </Select>
+          </SelectNext>
         </>
       ) : (
         <p data-testid="no-annotations-message">
diff --git a/src/sidebar/components/ShareDialog/ImportAnnotations.tsx b/src/sidebar/components/ShareDialog/ImportAnnotations.tsx
index 24aeaec8550..b36146ec393 100644
--- a/src/sidebar/components/ShareDialog/ImportAnnotations.tsx
+++ b/src/sidebar/components/ShareDialog/ImportAnnotations.tsx
@@ -1,4 +1,9 @@
-import { Button, CardActions, Link, Select } from '@hypothesis/frontend-shared';
+import {
+  Button,
+  CardActions,
+  Link,
+  SelectNext,
+} from '@hypothesis/frontend-shared';
 import { useCallback, useEffect, useId, useMemo, useState } from 'preact/hooks';
 
 import type { APIAnnotationData } from '../../../types/api';
@@ -10,6 +15,7 @@ import type { ImportAnnotationsService } from '../../services/import-annotations
 import { useSidebarStore } from '../../store';
 import CircularProgress from '../CircularProgress';
 import FileInput from './FileInput';
+import { UserAnnotationsListItem } from './UserAnnotationsListItem';
 
 export type ImportAnnotationsProps = {
   importAnnotationsService: ImportAnnotationsService;
@@ -32,9 +38,6 @@ function ImportAnnotations({
   );
   const [error, setError] = useState<string | null>(null);
 
-  // User whose annotations are going to be imported.
-  const [selectedUser, setSelectedUser] = useState<string | null>(null);
-
   const store = useSidebarStore();
   const currentUser = store.profile().userid;
 
@@ -59,6 +62,13 @@ function ImportAnnotations({
     [annotations, getDisplayName],
   );
 
+  // User whose annotations are going to be imported.
+  const [selectedUserId, setSelectedUserId] = useState<string | null>(null);
+  const selectedUser = useMemo(
+    () => userList?.find(user => user.userid === selectedUserId) ?? null,
+    [selectedUserId, userList],
+  );
+
   // Parse input file, extract annotations and update the user list.
   useEffect(() => {
     if (!currentUser || !file) {
@@ -66,7 +76,7 @@ function ImportAnnotations({
     }
     setAnnotations(null);
     setError(null);
-    setSelectedUser(null);
+    setSelectedUserId(null);
     readExportFile(file)
       .then(annotations => {
         setAnnotations(annotations);
@@ -74,7 +84,7 @@ function ImportAnnotations({
         // Pre-select the current user in the list, if at least one of the
         // annotations was authored by them.
         const userMatch = annotations.some(ann => ann.user === currentUser);
-        setSelectedUser(userMatch ? currentUser : null);
+        setSelectedUserId(userMatch ? currentUser : null);
       })
       .catch(err => {
         setError(err.message);
@@ -82,17 +92,11 @@ function ImportAnnotations({
   }, [currentUser, file]);
 
   let importAnnotations;
-  if (annotations && selectedUser && userList) {
+  if (selectedUser) {
     importAnnotations = async () => {
-      const userEntry = userList.find(item => item.userid === selectedUser);
-      /* istanbul ignore next - This should never be triggered */
-      if (!userEntry) {
-        return;
-      }
-
       // nb. In the event of an error, `import` will report that directly via
       // a toast message, so we don't do that ourselves.
-      importAnnotationsService.import(userEntry.annotations);
+      importAnnotationsService.import(selectedUser.annotations);
     };
   }
 
@@ -159,25 +163,24 @@ function ImportAnnotations({
           <label htmlFor={userSelectId} className="block font-medium">
             Select which user&apos;s annotations to import:
           </label>
-          <Select
-            id={userSelectId}
-            data-testid="user-list"
-            disabled={busy}
-            onChange={e =>
-              setSelectedUser((e.target as HTMLSelectElement).value || null)
+          <SelectNext
+            value={selectedUser}
+            onChange={newValue => setSelectedUserId(newValue?.userid ?? null)}
+            buttonId={userSelectId}
+            buttonContent={
+              selectedUser ? (
+                <UserAnnotationsListItem userAnnotations={selectedUser} />
+              ) : (
+                <span className="text-grey-6">Select a user...</span>
+              )
             }
           >
-            <option value="" selected={!selectedUser} />
             {userList.map(userInfo => (
-              <option
-                key={userInfo.userid}
-                value={userInfo.userid}
-                selected={userInfo.userid === selectedUser}
-              >
-                {userInfo.displayName} ({userInfo.annotations.length})
-              </option>
+              <SelectNext.Option key={userInfo.userid} value={userInfo}>
+                {() => <UserAnnotationsListItem userAnnotations={userInfo} />}
+              </SelectNext.Option>
             ))}
-          </Select>
+          </SelectNext>
         </>
       )}
       {error && (
diff --git a/src/sidebar/components/ShareDialog/UserAnnotationsListItem.tsx b/src/sidebar/components/ShareDialog/UserAnnotationsListItem.tsx
new file mode 100644
index 00000000000..597da241d75
--- /dev/null
+++ b/src/sidebar/components/ShareDialog/UserAnnotationsListItem.tsx
@@ -0,0 +1,21 @@
+import type { UserAnnotations } from '../../helpers/annotations-by-user';
+
+export type UserAnnotationsListItemProps = {
+  userAnnotations: Omit<UserAnnotations, 'userid'>;
+};
+
+/**
+ * UserAnnotations representation to use inside `SelectNext.Option`.
+ */
+export function UserAnnotationsListItem({
+  userAnnotations,
+}: UserAnnotationsListItemProps) {
+  return (
+    <div className="flex gap-x-2">
+      {userAnnotations.displayName}
+      <div className="rounded px-1 bg-grey-7 text-white">
+        {userAnnotations.annotations.length}
+      </div>
+    </div>
+  );
+}
diff --git a/src/sidebar/components/ShareDialog/test/ExportAnnotations-test.js b/src/sidebar/components/ShareDialog/test/ExportAnnotations-test.js
index f0be6b9437e..c0baff41c36 100644
--- a/src/sidebar/components/ShareDialog/test/ExportAnnotations-test.js
+++ b/src/sidebar/components/ShareDialog/test/ExportAnnotations-test.js
@@ -1,9 +1,11 @@
+import { SelectNext } from '@hypothesis/frontend-shared';
 import {
   checkAccessibility,
   mockImportedComponents,
   waitForElement,
 } from '@hypothesis/frontend-testing';
 import { mount } from 'enzyme';
+import { act } from 'preact/test-utils';
 
 import * as fixtures from '../../../test/annotation-fixtures';
 import ExportAnnotations, { $imports } from '../ExportAnnotations';
@@ -66,9 +68,12 @@ describe('ExportAnnotations', () => {
       '../../store': { useSidebarStore: () => fakeStore },
     });
 
-    // Restore this very simple component to get it test coverage
     $imports.$restore({
+      // Restore this very simple component to get it test coverage
       './LoadingSpinner': true,
+      // Restore UserAnnotationsListItem, as it's used as some buttons' content
+      // and is needed to make a11y tests pass
+      './UserAnnotationsListItem': true,
     });
   });
 
@@ -126,9 +131,21 @@ describe('ExportAnnotations', () => {
         },
       ],
       userEntries: [
-        { value: '', text: 'All annotations (3)' }, // "No user selected" entry
-        { value: 'acct:brian@example.com', text: 'Brian Smith (2)' },
-        { value: 'acct:john@example.com', text: 'John Smith (1)' },
+        {
+          // "No user selected" entry
+          displayName: 'All annotations',
+          annotationsCount: 3,
+        },
+        {
+          userid: 'acct:brian@example.com',
+          displayName: 'Brian Smith',
+          annotationsCount: 2,
+        },
+        {
+          userid: 'acct:john@example.com',
+          displayName: 'John Smith',
+          annotationsCount: 1,
+        },
       ],
     },
 
@@ -146,8 +163,16 @@ describe('ExportAnnotations', () => {
         },
       ],
       userEntries: [
-        { value: '', text: 'All annotations (1)' }, // "No user selected" entry
-        { value: 'acct:brian@example.com', text: 'Brian Smith (1)' },
+        {
+          // "No user selected" entry
+          displayName: 'All annotations',
+          annotationsCount: 1,
+        },
+        {
+          userid: 'acct:brian@example.com',
+          displayName: 'Brian Smith',
+          annotationsCount: 1,
+        },
       ],
     },
   ].forEach(({ annotations, userEntries }) => {
@@ -156,13 +181,16 @@ describe('ExportAnnotations', () => {
 
       const wrapper = createComponent();
 
-      const userList = await waitForElement(wrapper, 'Select');
-      const users = userList.find('option');
+      const userList = await waitForElement(wrapper, SelectNext);
+      const users = userList.find(SelectNext.Option);
       assert.equal(users.length, userEntries.length);
 
       for (const [i, entry] of userEntries.entries()) {
-        assert.equal(users.at(i).prop('value'), entry.value);
-        assert.equal(users.at(i).text(), entry.text);
+        const value = users.at(i).prop('value');
+
+        assert.equal(value.userid, entry.userid);
+        assert.equal(value.displayName, entry.displayName);
+        assert.equal(value.annotations.length, entry.annotationsCount);
       }
     });
   });
@@ -237,11 +265,15 @@ describe('ExportAnnotations', () => {
       const wrapper = createComponent();
 
       // Select the user whose annotations we want to export
-      const userList = await waitForElement(wrapper, 'Select');
-      userList.prop('onChange')({
-        target: {
-          value: 'acct:john@example.com',
-        },
+      const userList = await waitForElement(wrapper, SelectNext);
+      const option = userList
+        .find(SelectNext.Option)
+        .filterWhere(
+          option => option.prop('value').userid === 'acct:john@example.com',
+        )
+        .first();
+      act(() => {
+        userList.prop('onChange')(option.prop('value'));
       });
       wrapper.update();
 
diff --git a/src/sidebar/components/ShareDialog/test/ImportAnnotations-test.js b/src/sidebar/components/ShareDialog/test/ImportAnnotations-test.js
index fdddfdb7b78..ea5b9fc52f3 100644
--- a/src/sidebar/components/ShareDialog/test/ImportAnnotations-test.js
+++ b/src/sidebar/components/ShareDialog/test/ImportAnnotations-test.js
@@ -1,3 +1,4 @@
+import { SelectNext } from '@hypothesis/frontend-shared';
 import {
   checkAccessibility,
   waitFor,
@@ -99,8 +100,8 @@ describe('ImportAnnotations', () => {
       },
     ];
     selectFile(wrapper, annotations);
-    const userList = await waitForElement(wrapper, 'select');
-    assert.ok(userList.getDOMNode().value); // Current user should be auto-selected
+    const userList = await waitForElement(wrapper, SelectNext);
+    assert.ok(userList.prop('value')); // Current user should be auto-selected
 
     // Import button should be disabled since we don't have the things we need
     // to perform the import.
@@ -159,9 +160,16 @@ describe('ImportAnnotations', () => {
         },
       ],
       userEntries: [
-        { value: '', text: '' }, // "No user selected" entry
-        { value: 'acct:brian@example.com', text: 'Brian Smith (1)' },
-        { value: 'acct:john@example.com', text: 'John Smith (1)' },
+        {
+          userid: 'acct:brian@example.com',
+          displayName: 'Brian Smith',
+          annotationsCount: 1,
+        },
+        {
+          userid: 'acct:john@example.com',
+          displayName: 'John Smith',
+          annotationsCount: 1,
+        },
       ],
     },
 
@@ -178,9 +186,7 @@ describe('ImportAnnotations', () => {
           references: ['abc'],
         },
       ],
-      userEntries: [
-        { value: '', text: '' }, // "No user selected" entry
-      ],
+      userEntries: [],
     },
   ].forEach(({ annotations, userEntries }) => {
     it('displays user list when a valid file is selected', async () => {
@@ -188,13 +194,17 @@ describe('ImportAnnotations', () => {
 
       selectFile(wrapper, annotations);
 
-      const userList = await waitForElement(wrapper, 'Select');
-      const users = userList.find('option');
+      const userList = await waitForElement(wrapper, SelectNext);
+      const users = userList.find(SelectNext.Option);
+
       assert.equal(users.length, userEntries.length);
 
       for (const [i, entry] of userEntries.entries()) {
-        assert.equal(users.at(i).prop('value'), entry.value);
-        assert.equal(users.at(i).text(), entry.text);
+        const optionValue = users.at(i).prop('value');
+
+        assert.equal(optionValue.userid, entry.userid);
+        assert.equal(optionValue.displayName, entry.displayName);
+        assert.equal(optionValue.annotations.length, entry.annotationsCount);
       }
     });
   });
@@ -225,8 +235,8 @@ describe('ImportAnnotations', () => {
     ];
     selectFile(wrapper, annotations);
 
-    const userList = await waitForElement(wrapper, 'Select');
-    assert.equal(userList.getDOMNode().value, 'acct:john@example.com');
+    const userList = await waitForElement(wrapper, SelectNext);
+    assert.equal(userList.props().value.userid, 'acct:john@example.com');
   });
 
   it('does not select a user if no user matches logged-in user', async () => {
@@ -243,8 +253,8 @@ describe('ImportAnnotations', () => {
     ];
     selectFile(wrapper, annotations);
 
-    const userList = await waitForElement(wrapper, 'Select');
-    assert.equal(userList.getDOMNode().value, '');
+    const userList = await waitForElement(wrapper, SelectNext);
+    assert.equal(userList.prop('value'), null);
   });
 
   it('imports annotations when "Import" button is clicked', async () => {
@@ -281,9 +291,16 @@ describe('ImportAnnotations', () => {
 
     selectFile(wrapper, annotations);
 
-    const userList = await waitForElement(wrapper, 'select');
-    userList.getDOMNode().value = 'acct:brian@example.com';
-    userList.simulate('change');
+    const userList = await waitForElement(wrapper, SelectNext);
+    const option = userList
+      .find(SelectNext.Option)
+      .filterWhere(
+        option => option.prop('value').userid === 'acct:brian@example.com',
+      )
+      .first();
+
+    userList.prop('onChange')(option.prop('value'));
+    wrapper.update();
 
     const importButton = getImportButton(wrapper).getDOMNode();
     await waitFor(() => !importButton.disabled);
diff --git a/yarn.lock b/yarn.lock
index a514a27c048..66308abca7e 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -2059,15 +2059,15 @@ __metadata:
   languageName: node
   linkType: hard
 
-"@hypothesis/frontend-shared@npm:^6.5.0":
-  version: 6.8.0
-  resolution: "@hypothesis/frontend-shared@npm:6.8.0"
+"@hypothesis/frontend-shared@npm:^6.8.1":
+  version: 6.8.1
+  resolution: "@hypothesis/frontend-shared@npm:6.8.1"
   dependencies:
     highlight.js: ^11.6.0
     wouter-preact: ^2.10.0-alpha.1
   peerDependencies:
     preact: ^10.4.0
-  checksum: e2981d395929d9ddd5b6b78f35b4aebe5422ebbdaa48f6e65684cb1514534186e98559b2aadbaad77f3dc02652b73128c5bf3714d86b14f4104d5162d9d79466
+  checksum: d2f78b52c75930a9e4e337874835977b8569320ca64c2e75df125a9f45427fb4b93267d7b153ee3e2a7021ce9f49c54fcd0ad7a9cc44ad55cf531ddaaa184b7e
   languageName: node
   linkType: hard
 
@@ -7403,7 +7403,7 @@ __metadata:
     "@babel/preset-react": ^7.0.0
     "@babel/preset-typescript": ^7.16.7
     "@hypothesis/frontend-build": ^2.0.0
-    "@hypothesis/frontend-shared": ^6.5.0
+    "@hypothesis/frontend-shared": ^6.8.1
     "@hypothesis/frontend-testing": ^1.0.1
     "@npmcli/arborist": ^7.0.0
     "@octokit/rest": ^20.0.1