Use SelectNext for import and export user selectors

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",

src/sidebar/components/ShareDialog/ExportAnnotations.tsx

@@ -3,20 +3,22 @@ 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';
 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">

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,40 +62,41 @@ 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) {
       return;
     }
     setAnnotations(null);
     setError(null);
-    setSelectedUser(null);
+    setSelectedUserId(null);
     readExportFile(file)
       .then(annotations => {
         setAnnotations(annotations);
 
         // 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);
       });
   }, [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 && (

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

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();