From 0d7bcdd0776ebadbabe83fe67c929d6e20824695 Mon Sep 17 00:00:00 2001 From: Jon Willesen Date: Wed, 2 Oct 2019 10:46:08 -0600 Subject: [PATCH] refactor alerts in direct share course tray This is a preparatory commit for reusing the tray for a content share import. It also includes a minor change in behavior to the tray: when the copy operation has started, the "copy" and "cancel" buttons are replaced with a single "close" button. The mechanism for doing these particular screen reader alerts has also changed to something that should be more reliable. Instead of using the static alert region on the page, this simply adds the screen reader alert properties to the alert text. Modern screen readers seem to handle this better than the static alert div. refs ADMIN-2912 flag=direct_share test plan: - should still be able to copy a discussion from one course to another as before, with the changes to the button behavior specified above. - screen reader alerts for the operation status should still work. Change-Id: I46395b0c70cd111f455feac7f7d8d44b785552d1 Reviewed-on: https://gerrit.instructure.com/211843 Tested-by: Jenkins Reviewed-by: Charley Kline QA-Review: Charley Kline Product-Review: Jon Willesen --- .prettierwhitelist.js | 5 +- .../shared/components/CanvasInlineAlert.js | 51 ++++++++ .../__tests__/CanvasInlineAlert.test.js | 41 ++++++ .../direct_share/DirectShareCoursePanel.js | 86 ++++--------- .../DirectShareOperationStatus.js | 75 +++++++++++ .../__tests__/DirectShareCoursePanel.test.js | 38 +++--- .../DirectShareOperationStatus.test.js | 120 ++++++++++++++++++ 7 files changed, 336 insertions(+), 80 deletions(-) create mode 100644 app/jsx/shared/components/CanvasInlineAlert.js create mode 100644 app/jsx/shared/components/__tests__/CanvasInlineAlert.test.js create mode 100644 app/jsx/shared/direct_share/DirectShareOperationStatus.js create mode 100644 app/jsx/shared/direct_share/__tests__/DirectShareOperationStatus.test.js diff --git a/.prettierwhitelist.js b/.prettierwhitelist.js index 14cd00d3b5a..12e81143e24 100644 --- a/.prettierwhitelist.js +++ b/.prettierwhitelist.js @@ -10,7 +10,7 @@ function appAndSpecFilesFor(path) { // or if there is a folder of code that your team controls that you want // to start ensuring conforms to prettier, add it to this array to opt-in // now to conform to prettier. -const PRETTIER_WHITELIST = module.exports = [ +const PRETTIER_WHITELIST = (module.exports = [ './*.js', 'app/jsx/*.js', 'frontend_build/**/*.js', @@ -40,6 +40,7 @@ const PRETTIER_WHITELIST = module.exports = [ appAndSpecDirsFor('login'), appAndSpecDirsFor('permissions'), appAndSpecDirsFor('shared/components'), + appAndSpecDirsFor('shared/direct_share'), appAndSpecDirsFor('speed_grader'), appAndSpecDirsFor('theme_editor') -] +]) diff --git a/app/jsx/shared/components/CanvasInlineAlert.js b/app/jsx/shared/components/CanvasInlineAlert.js new file mode 100644 index 00000000000..7e912c08859 --- /dev/null +++ b/app/jsx/shared/components/CanvasInlineAlert.js @@ -0,0 +1,51 @@ +/* + * Copyright (C) 2019 - present Instructure, Inc. + * + * This file is part of Canvas. + * + * Canvas is free software: you can redistribute it and/or modify it under + * the terms of the GNU Affero General Public License as published by the Free + * Software Foundation, version 3 of the License. + * + * Canvas is distributed in the hope that it will be useful, but WITHOUT ANY + * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR + * A PARTICULAR PURPOSE. See the GNU Affero General Public License for more + * details. + * + * You should have received a copy of the GNU Affero General Public License along + * with this program. If not, see . + */ + +import React from 'react' +import {bool, node, oneOf} from 'prop-types' +import {Alert} from '@instructure/ui-alerts' +import {ScreenReaderContent} from '@instructure/ui-a11y-content' + +CanvasInlineAlert.propTypes = { + liveAlert: bool, + screenReaderOnly: bool, + politeness: oneOf(['assertive', 'polite']), + children: node +} + +export default function CanvasInlineAlert({ + children, + liveAlert, + screenReaderOnly, + politeness = 'assertive', + ...alertProps +}) { + let body = children + if (liveAlert || screenReaderOnly) { + body = ( + + {body} + + ) + } + if (screenReaderOnly) { + body = {body} + } + + return {body} +} diff --git a/app/jsx/shared/components/__tests__/CanvasInlineAlert.test.js b/app/jsx/shared/components/__tests__/CanvasInlineAlert.test.js new file mode 100644 index 00000000000..ca184f203b8 --- /dev/null +++ b/app/jsx/shared/components/__tests__/CanvasInlineAlert.test.js @@ -0,0 +1,41 @@ +/* + * Copyright (C) 2019 - present Instructure, Inc. + * + * This file is part of Canvas. + * + * Canvas is free software: you can redistribute it and/or modify it under + * + * the terms of the GNU Affero General Public License as published by the Free + * Software Foundation, version 3 of the License. + * + * Canvas is distributed in the hope that it will be useful, but WITHOUT ANY + * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR + * A PARTICULAR PURPOSE. See the GNU Affero General Public License for more + * details. + * + * You should have received a copy of the GNU Affero General Public License along + * with this program. If not, see . + */ + +import React from 'react' +import {render} from '@testing-library/react' +import CanvasInlineAlert from '../CanvasInlineAlert' + +describe('CanvasInlineAlert', () => { + it('renders a basic alert with no sr features', () => { + const {getByText} = render(alert message) + expect(getByText('alert message')).toBeInTheDocument() + expect(document.querySelector('[role="alert"]')).toBeNull() + }) + + it('renders an alert with sr features', () => { + render(alert message) + expect(document.querySelector('[role="alert"]')).toBeInTheDocument() + }) + + it('renders an sr-only alert', () => { + render(alert message) + expect(document.querySelector('[role="alert"]')).toBeInTheDocument() + // There's no great way to tell if instui has done its ScreenReaderContent thing + }) +}) diff --git a/app/jsx/shared/direct_share/DirectShareCoursePanel.js b/app/jsx/shared/direct_share/DirectShareCoursePanel.js index 6cf7f2b45ea..20aaf149ca9 100644 --- a/app/jsx/shared/direct_share/DirectShareCoursePanel.js +++ b/app/jsx/shared/direct_share/DirectShareCoursePanel.js @@ -21,14 +21,13 @@ import I18n from 'i18n!direct_share_course_panel' import React, {useState} from 'react' import {func, string} from 'prop-types' -import {Alert} from '@instructure/ui-alerts' import {Button} from '@instructure/ui-buttons' import {Flex} from '@instructure/ui-layout' -import {Spinner} from '@instructure/ui-elements' import doFetchApi from 'jsx/shared/effects/doFetchApi' import contentSelectionShape from 'jsx/shared/proptypes/contentSelection' import ManagedCourseSelector from '../components/ManagedCourseSelector' +import DirectShareOperationStatus from './DirectShareOperationStatus' // eventually this will have options for where to place the item in the new course. // for now, it just has the selector plus some buttons @@ -39,79 +38,44 @@ DirectShareCoursePanel.propTypes = { onCancel: func } -function getLiveRegion() { - return document.querySelector('#flash_screenreader_holder') -} - export default function DirectShareCoursePanel({sourceCourseId, contentSelection, onCancel}) { const [selectedCourse, setSelectedCourse] = useState(null) - const [postStatus, setPostStatus] = useState(null) + const [startCopyOperationPromise, setStartCopyOperationPromise] = useState(null) function startCopyOperation() { - return doFetchApi({ - method: 'POST', - path: `/api/v1/courses/${selectedCourse.id}/content_migrations`, - body: { - migration_type: 'course_copy_importer', - select: contentSelection, - settings: {source_course_id: sourceCourseId} - }, - }) - } - - - function handleStart() { - setPostStatus('starting') - startCopyOperation() - .then(() => { - setPostStatus('success') - }) - .catch(err => { - console.error(err) // eslint-disable-line no-console - if (err.response) console.error(err.response) // eslint-disable-line no-console - setPostStatus('error') - }) - } - - let alert = null - const alertProps = { - margin: 'small 0', - liveRegion: getLiveRegion - } - if (postStatus === 'error') { - alert = ( - - {I18n.t('There was a problem starting the copy operation')} - - ) - } else if (postStatus === 'success') { - alert = ( - - {I18n.t('Copy operation started successfully')} - - ) - } else if (postStatus === 'starting') { - alert = ( - - {I18n.t('Starting copy operation')} - - + setStartCopyOperationPromise( + doFetchApi({ + method: 'POST', + path: `/api/v1/courses/${selectedCourse.id}/content_migrations`, + body: { + migration_type: 'course_copy_importer', + select: contentSelection, + settings: {source_course_id: sourceCourseId} + } + }) ) } - const copyButtonDisabled = selectedCourse === null || postStatus !== null + const copyButton = startCopyOperationPromise ? null : ( + + ) return ( <> - {alert} + - + {copyButton} diff --git a/app/jsx/shared/direct_share/DirectShareOperationStatus.js b/app/jsx/shared/direct_share/DirectShareOperationStatus.js new file mode 100644 index 00000000000..c829fe5dffa --- /dev/null +++ b/app/jsx/shared/direct_share/DirectShareOperationStatus.js @@ -0,0 +1,75 @@ +/* + * Copyright (C) 2019 - present Instructure, Inc. + * + * This file is part of Canvas. + * + * Canvas is free software: you can redistribute it and/or modify it under + * the terms of the GNU Affero General Public License as published by the Free + * Software Foundation, version 3 of the License. + * + * Canvas is distributed in the hope that it will be useful, but WITHOUT ANY + * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR + * A PARTICULAR PURPOSE. See the GNU Affero General Public License for more + * details. + * + * You should have received a copy of the GNU Affero General Public License along + * with this program. If not, see . + */ + +import React, {useRef, useState} from 'react' +import CanvasInlineAlert from 'jsx/shared/components/CanvasInlineAlert' +import {Spinner} from '@instructure/ui-elements' + +export default function DirectShareOperationStatus({promise, startingMsg, successMsg, errorMsg}) { + const [operationStatus, setOperationStatus] = useState('starting') + const previousPromise = useRef(null) + + if (previousPromise.current !== promise) { + previousPromise.current = promise + setOperationStatus('starting') + if (promise) { + promise + .then(() => { + if (promise === previousPromise.current) { + setOperationStatus('success') + } + }) + .catch(err => { + if (promise === previousPromise.current) { + console.error(err) // eslint-disable-line no-console + if (err && err.response) console.error(err.response) // eslint-disable-line no-console + setOperationStatus('error') + } + }) + } + } + + let alert + const alertProps = { + liveAlert: true, + margin: 'small 0' + } + + if (operationStatus === 'error') { + alert = ( + + {errorMsg} + + ) + } else if (operationStatus === 'success') { + alert = ( + + {successMsg} + + ) + } else if (operationStatus === 'starting') { + alert = ( + + {startingMsg} + + + ) + } + + return promise ? alert : null +} diff --git a/app/jsx/shared/direct_share/__tests__/DirectShareCoursePanel.test.js b/app/jsx/shared/direct_share/__tests__/DirectShareCoursePanel.test.js index 0ea0f4c9330..5fc81d4a45a 100644 --- a/app/jsx/shared/direct_share/__tests__/DirectShareCoursePanel.test.js +++ b/app/jsx/shared/direct_share/__tests__/DirectShareCoursePanel.test.js @@ -87,21 +87,22 @@ describe('DirectShareCoursePanel', () => { }) it('starts a copy operation and reports status', async () => { - fetchMock.postOnce( - 'path:/api/v1/courses/abc/content_migrations', - {id: '8', workflow_state: 'running'} - ) - const {getByText, getAllByText, getByLabelText} = render( + fetchMock.postOnce('path:/api/v1/courses/abc/content_migrations', { + id: '8', + workflow_state: 'running' + }) + const {getByText, getByLabelText, queryByText} = render( ) + /> + ) const input = getByLabelText(/select a course/i) fireEvent.click(input) fireEvent.click(getByText('abc')) - const copyButton = getByText(/copy/i).closest('button') - fireEvent.click(copyButton) - expect(copyButton.getAttribute('disabled')).toBe('') + fireEvent.click(getByText(/copy/i)) + expect(queryByText('Copy')).toBeNull() + expect(getByText('Close')).toBeInTheDocument() const [, fetchOptions] = fetchMock.lastCall() expect(fetchOptions.method).toBe('POST') expect(JSON.parse(fetchOptions.body)).toMatchObject({ @@ -109,10 +110,11 @@ describe('DirectShareCoursePanel', () => { select: {discussion_topics: ['1123']}, settings: {source_course_id: '42'} }) - expect(getAllByText(/start/i)).toHaveLength(2) + expect(getByText(/start/i)).toBeInTheDocument() await act(() => fetchMock.flush(true)) - expect(getAllByText(/success/)).toHaveLength(2) - expect(copyButton.getAttribute('disabled')).toBe('') + expect(getByText(/success/)).toBeInTheDocument() + expect(queryByText('Copy')).toBeNull() + expect(getByText('Close')).toBeInTheDocument() }) describe('errors', () => { @@ -126,15 +128,17 @@ describe('DirectShareCoursePanel', () => { it('reports an error if the fetch fails', async () => { fetchMock.postOnce('path:/api/v1/courses/abc/content_migrations', 400) - const {getByText, getAllByText, getByLabelText} = render() + const {getByText, getByLabelText, queryByText} = render( + + ) const input = getByLabelText(/select a course/i) fireEvent.click(input) fireEvent.click(getByText('abc')) - const copyButton = getByText(/copy/i).closest('button') - fireEvent.click(copyButton) + fireEvent.click(getByText('Copy')) await act(() => fetchMock.flush(true)) - expect(getAllByText(/problem/i)).toHaveLength(2) - expect(copyButton.getAttribute('disabled')).toBe('') + expect(getByText(/problem/i)).toBeInTheDocument() + expect(queryByText('Copy')).toBeNull() + expect(getByText('Close')).toBeInTheDocument() }) }) }) diff --git a/app/jsx/shared/direct_share/__tests__/DirectShareOperationStatus.test.js b/app/jsx/shared/direct_share/__tests__/DirectShareOperationStatus.test.js new file mode 100644 index 00000000000..6a4ffd650ac --- /dev/null +++ b/app/jsx/shared/direct_share/__tests__/DirectShareOperationStatus.test.js @@ -0,0 +1,120 @@ +/* + * Copyright (C) 2019 - present Instructure, Inc. + * + * This file is part of Canvas. + * + * Canvas is free software: you can redistribute it and/or modify it under + * + * the terms of the GNU Affero General Public License as published by the Free + * Software Foundation, version 3 of the License. + * + * Canvas is distributed in the hope that it will be useful, but WITHOUT ANY + * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR + * A PARTICULAR PURPOSE. See the GNU Affero General Public License for more + * details. + * + * You should have received a copy of the GNU Affero General Public License along + * with this program. If not, see . + */ + +import React from 'react' +import {render} from '@testing-library/react' +import DirectShareOperationStatus from '../DirectShareOperationStatus' + +describe('DirectShareOperationStatus', () => { + it('shows nothing if there is no promise', () => { + const {queryByText} = render( + + ) + expect(queryByText(/starting|success|error/)).toBeNull() + }) + + it('shows the starting message when the promise is not resolved', () => { + const promise = new Promise(() => {}) + const {getByText, queryByText} = render( + + ) + expect(getByText('starting')).toBeInTheDocument() + expect(queryByText(/success|error/)).toBeNull() + }) + + it('does an sr alert', () => { + const promise = new Promise(() => {}) + render( + + ) + expect(document.querySelector('[role="alert"]')).toBeInTheDocument() + }) + + it('shows success when promise is fulfilled', async () => { + const promise = Promise.resolve() + const {findByText, queryByText} = render( + + ) + expect(await findByText('success')).toBeInTheDocument() + expect(queryByText(/starting|error/)).toBeNull() + }) + + describe('errors', () => { + beforeEach(() => jest.spyOn(console, 'error').mockImplementation(() => {})) + afterEach(() => console.error.mockRestore()) // eslint-disable-line no-console + + it('shows error when promise is rejected', async () => { + const promise = Promise.reject() + const {findByText, queryByText} = render( + + ) + expect(await findByText('error')).toBeInTheDocument() + expect(queryByText(/starting|success/)).toBeNull() + expect(console.error).toHaveBeenCalled() // eslint-disable-line no-console + }) + }) + + it('resets when it receives a new promise', async () => { + const promise = Promise.resolve() + const {findByText, getByText, rerender} = render( + + ) + expect(await findByText('success')).toBeInTheDocument() + const newPromise = new Promise(() => {}) + rerender( + + ) + expect(getByText('starting')).toBeInTheDocument() + }) +})