From e036588fac4f60ef5ea6abcb7e42964c4af66ccb Mon Sep 17 00:00:00 2001 From: Alejandro Celaya 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 | 53 +++++++++------ .../ShareDialog/ImportAnnotations.tsx | 67 ++++++++++--------- .../ShareDialog/UserAnnotationsListItem.tsx | 21 ++++++ .../test/ExportAnnotations-test.js | 62 ++++++++++++----- .../test/ImportAnnotations-test.js | 55 +++++++++------ yarn.lock | 4 +- 7 files changed, 174 insertions(+), 90 deletions(-) create mode 100644 src/sidebar/components/ShareDialog/UserAnnotationsListItem.tsx diff --git a/package.json b/package.json index 4eb0f41fd94..33c101414cc 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.0", "@npmcli/arborist": "^7.0.0", "@octokit/rest": "^20.0.1", "@rollup/plugin-babel": "^6.0.0", diff --git a/src/sidebar/components/ShareDialog/ExportAnnotations.tsx b/src/sidebar/components/ShareDialog/ExportAnnotations.tsx index 681d239b86c..8b2927c57c6 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 { 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,20 @@ 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 = useMemo( + () => ({ + annotations: exportableAnnotations, + displayName: 'All annotations', + }), + [exportableAnnotations], + ); + const [selectedUser, setSelectedUser] = useState( + // Try to preselect current user + userList.find(userInfo => userInfo.userid === currentUser) ?? + allAnnotationsOption, + ); const draftCount = store.countDrafts(); @@ -76,8 +89,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); @@ -132,25 +144,24 @@ function ExportAnnotations({ - + ) : (

diff --git a/src/sidebar/components/ShareDialog/ImportAnnotations.tsx b/src/sidebar/components/ShareDialog/ImportAnnotations.tsx index 24aeaec8550..54c38291530 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(null); - // User whose annotations are going to be imported. - const [selectedUser, setSelectedUser] = useState(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(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,22 +92,15 @@ 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); }; } const fileInputId = useId(); - const userSelectId = useId(); if (!currentUser) { // TODO - Make "Log in" a link. @@ -156,28 +159,28 @@ function ImportAnnotations({ {userList && ( <> -

Select which user's annotations to import: - - + )} {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 representation to use inside `SelectNext.Option`. + */ +export function UserAnnotationsListItem({ + userAnnotations, +}: UserAnnotationsListItemProps) { + return ( +

+ {userAnnotations.displayName} +
+ {userAnnotations.annotations.length} +
+
+ ); +} diff --git a/src/sidebar/components/ShareDialog/test/ExportAnnotations-test.js b/src/sidebar/components/ShareDialog/test/ExportAnnotations-test.js index 56df266c0ba..b27f81ae7c7 100644 --- a/src/sidebar/components/ShareDialog/test/ExportAnnotations-test.js +++ b/src/sidebar/components/ShareDialog/test/ExportAnnotations-test.js @@ -1,4 +1,6 @@ +import { SelectNext } from '@hypothesis/frontend-shared'; import { mount } from 'enzyme'; +import { act } from 'preact/test-utils'; import { checkAccessibility } from '../../../../test-util/accessibility'; import { mockImportedComponents } from '../../../../test-util/mock-imported-components'; @@ -64,9 +66,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 can make a11y tests fail + './UserAnnotationsListItem': true, }); }); @@ -124,9 +129,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, + }, ], }, @@ -144,8 +161,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 }) => { @@ -154,13 +179,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); } }); }); @@ -235,11 +263,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 a3cc34856f0..f95e467fdfd 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 { mount } from 'enzyme'; import { checkAccessibility } from '../../../../test-util/accessibility'; @@ -96,8 +97,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. @@ -156,9 +157,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, + }, ], }, @@ -175,9 +183,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 () => { @@ -185,13 +191,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); } }); }); @@ -222,8 +232,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 () => { @@ -240,8 +250,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 () => { @@ -278,9 +288,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 32974a25f93..32821a201a2 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2059,7 +2059,7 @@ __metadata: languageName: node linkType: hard -"@hypothesis/frontend-shared@npm:^6.5.0": +"@hypothesis/frontend-shared@npm:^6.8.0": version: 6.8.0 resolution: "@hypothesis/frontend-shared@npm:6.8.0" dependencies: @@ -7392,7 +7392,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.0 "@npmcli/arborist": ^7.0.0 "@octokit/rest": ^20.0.1 "@rollup/plugin-babel": ^6.0.0