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..33c3acc4822 100644 --- a/src/sidebar/components/ShareDialog/ExportAnnotations.tsx +++ b/src/sidebar/components/ShareDialog/ExportAnnotations.tsx @@ -3,13 +3,15 @@ import { CardActions, Link, Input, - Select, + SelectNext, } from '@hypothesis/frontend-shared'; +import classnames from 'classnames'; 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 +19,7 @@ import type { AnnotationsExporter } from '../../services/annotations-exporter'; import type { ToastMessengerService } from '../../services/toast-messenger'; import { useSidebarStore } from '../../store'; import LoadingSpinner from './LoadingSpinner'; +import { UserAnnotationsContent } from './UserAnnotationsContent'; export type ExportAnnotationsProps = { // injected @@ -51,9 +54,21 @@ 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: UserAnnotations = useMemo( + () => ({ + annotations: exportableAnnotations, + userid: '__all__', + 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 +91,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 +146,29 @@ function ExportAnnotations({ - + ) : (

diff --git a/src/sidebar/components/ShareDialog/ImportAnnotations.tsx b/src/sidebar/components/ShareDialog/ImportAnnotations.tsx index 24aeaec8550..30e21c605d1 100644 --- a/src/sidebar/components/ShareDialog/ImportAnnotations.tsx +++ b/src/sidebar/components/ShareDialog/ImportAnnotations.tsx @@ -1,8 +1,15 @@ -import { Button, CardActions, Link, Select } from '@hypothesis/frontend-shared'; +import { + Button, + CardActions, + Link, + SelectNext, +} from '@hypothesis/frontend-shared'; +import classnames from 'classnames'; import { useCallback, useEffect, useId, useMemo, useState } from 'preact/hooks'; 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 { readExportFile } from '../../helpers/import'; import { withServices } from '../../service-context'; @@ -10,6 +17,7 @@ import type { ImportAnnotationsService } from '../../services/import-annotations import { useSidebarStore } from '../../store'; import CircularProgress from '../CircularProgress'; import FileInput from './FileInput'; +import { UserAnnotationsContent } from './UserAnnotationsContent'; export type ImportAnnotationsProps = { importAnnotationsService: ImportAnnotationsService; @@ -33,7 +41,9 @@ function ImportAnnotations({ const [error, setError] = useState(null); // User whose annotations are going to be imported. - const [selectedUser, setSelectedUser] = useState(null); + const [selectedUser, setSelectedUser] = useState( + null, + ); const store = useSidebarStore(); const currentUser = store.profile().userid; @@ -59,6 +69,11 @@ function ImportAnnotations({ [annotations, getDisplayName], ); + // Try to pre-select current user + if (userList && !selectedUser) { + setSelectedUser(userList.find(user => user.userid === currentUser) ?? null); + } + // Parse input file, extract annotations and update the user list. useEffect(() => { if (!currentUser || !file) { @@ -68,36 +83,22 @@ function ImportAnnotations({ setError(null); setSelectedUser(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); - }) + .then(setAnnotations) .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); }; } const fileInputId = useId(); - const userSelectId = useId(); if (!currentUser) { // TODO - Make "Log in" a link. @@ -156,28 +157,33 @@ function ImportAnnotations({ {userList && ( <> -

Select which user's annotations to import: - - + )} {error && ( diff --git a/src/sidebar/components/ShareDialog/UserAnnotationsContent.tsx b/src/sidebar/components/ShareDialog/UserAnnotationsContent.tsx new file mode 100644 index 00000000000..abbaa8876d2 --- /dev/null +++ b/src/sidebar/components/ShareDialog/UserAnnotationsContent.tsx @@ -0,0 +1,22 @@ +import classnames from 'classnames'; + +import type { UserAnnotations } from '../../helpers/annotations-by-user'; + +export type UserAnnotationsContentProps = { + userAnnotations: UserAnnotations; + classes?: string; +}; + +export function UserAnnotationsContent({ + userAnnotations, + classes, +}: UserAnnotationsContentProps) { + 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..a6c0e8c43b5 100644 --- a/src/sidebar/components/ShareDialog/test/ExportAnnotations-test.js +++ b/src/sidebar/components/ShareDialog/test/ExportAnnotations-test.js @@ -1,3 +1,4 @@ +import { SelectNext } from '@hypothesis/frontend-shared'; import { mount } from 'enzyme'; import { checkAccessibility } from '../../../../test-util/accessibility'; @@ -5,6 +6,7 @@ import { mockImportedComponents } from '../../../../test-util/mock-imported-comp import { waitForElement } from '../../../../test-util/wait'; import * as fixtures from '../../../test/annotation-fixtures'; import ExportAnnotations, { $imports } from '../ExportAnnotations'; +import { UserAnnotationsContent } from '../UserAnnotationsContent'; describe('ExportAnnotations', () => { let fakeStore; @@ -62,6 +64,9 @@ describe('ExportAnnotations', () => { suggestedFilename: fakeSuggestedFilename, }, '../../store': { useSidebarStore: () => fakeStore }, + // Do not mock UserAnnotationsContent, as it's used as some buttons' + // content and can make a11y tests fail + './UserAnnotationsContent': { UserAnnotationsContent }, }); // Restore this very simple component to get it test coverage @@ -124,9 +129,22 @@ 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 + userid: '__all__', + 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 +162,17 @@ 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 + userid: '__all__', + displayName: 'All annotations', + annotationsCount: 1, + }, + { + userid: 'acct:brian@example.com', + displayName: 'Brian Smith', + annotationsCount: 1, + }, ], }, ].forEach(({ annotations, userEntries }) => { @@ -154,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); } }); }); @@ -235,12 +265,14 @@ 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(); + userList.prop('onChange')(option.prop('value')); wrapper.update(); submitExportForm(wrapper); 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 b1de5a51e49..d6367b00353 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.7.0 - resolution: "@hypothesis/frontend-shared@npm:6.7.0" +"@hypothesis/frontend-shared@npm:^6.8.0": + version: 6.8.0 + resolution: "@hypothesis/frontend-shared@npm:6.8.0" dependencies: highlight.js: ^11.6.0 wouter-preact: ^2.10.0-alpha.1 peerDependencies: preact: ^10.4.0 - checksum: cd4ec998cfeae73ab7163a9cea599c7ecb6c5508fa12aa18a5722a85788148a19aca02514eaf3d6032dac822c3cf433b313477fe5173172a55e053658ebcf984 + checksum: e2981d395929d9ddd5b6b78f35b4aebe5422ebbdaa48f6e65684cb1514534186e98559b2aadbaad77f3dc02652b73128c5bf3714d86b14f4104d5162d9d79466 languageName: node linkType: hard @@ -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