From 65dda8d0921e090b6ff739e6894690f3853f1974 Mon Sep 17 00:00:00 2001 From: Kristin Aoki <42981026+KristinAoki@users.noreply.github.com> Date: Mon, 31 Jul 2023 17:23:07 -0400 Subject: [PATCH] fix: ui bugs (#542) --- src/advanced-settings/AdvancedSettings.jsx | 120 ++++++++--------- .../AdvancedSettings.test.jsx | 49 +++++-- .../__mocks__/advancedSettings.js | 7 + src/advanced-settings/data/thunks.js | 15 ++- .../scss/AdvancedSettings.scss | 56 ++++++-- .../setting-card/SettingCard.jsx | 126 ++++++++++++------ .../setting-card/SettingCard.test.jsx | 42 +++++- src/custom-pages/CustomPages.jsx | 5 +- 8 files changed, 282 insertions(+), 138 deletions(-) diff --git a/src/advanced-settings/AdvancedSettings.jsx b/src/advanced-settings/AdvancedSettings.jsx index 3e3f71fe64..2d72cf9c6f 100644 --- a/src/advanced-settings/AdvancedSettings.jsx +++ b/src/advanced-settings/AdvancedSettings.jsx @@ -2,7 +2,7 @@ import React, { useEffect, useState } from 'react'; import PropTypes from 'prop-types'; import { useDispatch, useSelector } from 'react-redux'; import { - Container, Button, Layout, StatefulButton, + Container, Button, Layout, StatefulButton, TransitionReplace, } from '@edx/paragon'; import { CheckCircle, Info, Warning } from '@edx/paragon/icons'; import { FormattedMessage, injectIntl, intlShape } from '@edx/frontend-platform/i18n'; @@ -25,10 +25,6 @@ import messages from './messages'; import ModalError from './modal-error/ModalError'; const AdvancedSettings = ({ intl, courseId }) => { - const advancedSettingsData = useSelector(getCourseAppSettings); - const savingStatus = useSelector(getSavingStatus); - const proctoringExamErrors = useSelector(getProctoringExamErrors); - const settingsWithSendErrors = useSelector(getSendRequestErrors) || {}; const dispatch = useDispatch(); const [saveSettingsPrompt, showSaveSettingsPrompt] = useState(false); const [showDeprecated, setShowDeprecated] = useState(false); @@ -36,10 +32,21 @@ const AdvancedSettings = ({ intl, courseId }) => { const [editedSettings, setEditedSettings] = useState({}); const [errorFields, setErrorFields] = useState([]); const [showSuccessAlert, setShowSuccessAlert] = useState(false); - const loadingSettingsStatus = useSelector(getLoadingStatus); const [isQueryPending, setIsQueryPending] = useState(false); const [isEditableState, setIsEditableState] = useState(false); const [hasInternetConnectionError, setInternetConnectionError] = useState(false); + + useEffect(() => { + dispatch(fetchCourseAppSettings(courseId)); + dispatch(fetchProctoringExamErrors(courseId)); + }, [courseId]); + + const advancedSettingsData = useSelector(getCourseAppSettings); + const savingStatus = useSelector(getSavingStatus); + const proctoringExamErrors = useSelector(getProctoringExamErrors); + const settingsWithSendErrors = useSelector(getSendRequestErrors) || {}; + const loadingSettingsStatus = useSelector(getLoadingStatus); + const isLoading = loadingSettingsStatus === RequestStatus.IN_PROGRESS; const updateSettingsButtonState = { labels: { @@ -49,20 +56,14 @@ const AdvancedSettings = ({ intl, courseId }) => { disabledStates: ['pending'], }; - useEffect(() => { - dispatch(fetchCourseAppSettings(courseId)); - dispatch(fetchProctoringExamErrors(courseId)); - }, [courseId]); - useEffect(() => { if (savingStatus === RequestStatus.SUCCESSFUL) { setIsQueryPending(false); setShowSuccessAlert(true); + setIsEditableState(false); + setTimeout(() => setShowSuccessAlert(false), 15000); window.scrollTo({ top: 0, behavior: 'smooth' }); - - if (!isEditableState) { - showSaveSettingsPrompt(false); - } + showSaveSettingsPrompt(false); } else if (savingStatus === RequestStatus.FAILED && !hasInternetConnectionError) { setErrorFields(settingsWithSendErrors); showErrorModal(true); @@ -81,26 +82,11 @@ const AdvancedSettings = ({ intl, courseId }) => { ); } - const handleSettingChange = (e, settingName) => { - const { value } = e.target; - if (!saveSettingsPrompt) { - showSaveSettingsPrompt(true); - } - setIsEditableState(true); - setShowSuccessAlert(false); - setEditedSettings((prevEditedSettings) => ({ - ...prevEditedSettings, - [settingName]: value, - })); - }; - const handleResetSettingsValues = () => { setIsEditableState(false); showErrorModal(false); setEditedSettings({}); showSaveSettingsPrompt(false); - setInternetConnectionError(false); - setIsQueryPending(false); }; const handleSettingBlur = () => { @@ -111,9 +97,7 @@ const AdvancedSettings = ({ intl, courseId }) => { const isValid = validateAdvancedSettingsData(editedSettings, setErrorFields, setEditedSettings); if (isValid) { setIsQueryPending(true); - setIsEditableState(false); } else { - setIsQueryPending(false); showSaveSettingsPrompt(false); showErrorModal(!errorModal); } @@ -123,7 +107,6 @@ const AdvancedSettings = ({ intl, courseId }) => { setInternetConnectionError(true); showSaveSettingsPrompt(false); setShowSuccessAlert(false); - setIsQueryPending(false); }; const handleQueryProcessing = () => { @@ -132,15 +115,13 @@ const AdvancedSettings = ({ intl, courseId }) => { }; const handleManuallyChangeClick = (setToState) => { - setIsEditableState(true); showErrorModal(setToState); showSaveSettingsPrompt(true); - setIsQueryPending(false); }; return ( <> - +
{(proctoringExamErrors?.length > 0) && ( { aria-describedby={intl.formatMessage(messages.alertProctoringDescribedby)} /> )} -
+
{
- Warning: }} - /> - )} - /> +
+ Warning: }} + /> +
    - {Object.keys(advancedSettingsData).sort().map((settingName) => { + {Object.keys(advancedSettingsData).map((settingName) => { const settingData = advancedSettingsData[settingName]; - const editedValue = editedSettings[settingName] !== undefined - ? editedSettings[settingName] : JSON.stringify(settingData.value, null, 4); - + if (settingData.deprecated && !showDeprecated) { + return null; + } return ( handleSettingChange(e, settingName)} - showDeprecated={showDeprecated} name={settingName} - value={editedValue} + showSaveSettingsPrompt={showSaveSettingsPrompt} + saveSettingsPrompt={saveSettingsPrompt} + setEdited={setEditedSettings} handleBlur={handleSettingBlur} + isEditableState={isEditableState} + setIsEditableState={setIsEditableState} /> ); })} @@ -233,7 +221,7 @@ const AdvancedSettings = ({ intl, courseId }) => {
- {!isEditableState && ( + {isQueryPending && ( ', () => { }); }); it('should render setting element', async () => { - const { getByText } = render(); + const { getByText, queryByText } = render(); await waitFor(() => { const advancedModuleListTitle = getByText(/Advanced Module List/i); expect(advancedModuleListTitle).toBeInTheDocument(); + expect(queryByText('Certificate web/html view enabled')).toBeNull(); }); }); it('should change to onŠ”hange', async () => { @@ -112,24 +115,50 @@ describe('', () => { fireEvent.click(showDeprecatedItemsBtn); expect(getByText(/Hide Deprecated Settings/i)).toBeInTheDocument(); }); + expect(getByText('Certificate web/html view enabled')).toBeInTheDocument(); }); it('should reset to default value on click on Cancel button', async () => { const { getByLabelText, getByText } = render(); + let textarea; await waitFor(() => { - const textarea = getByLabelText(/Advanced Module List/i); - fireEvent.change(textarea, { target: { value: '[3, 2, 1]' } }); - fireEvent.click(getByText(messages.buttonCancelText.defaultMessage)); - expect(textarea.value).toBe('[]'); + textarea = getByLabelText(/Advanced Module List/i); }); + fireEvent.change(textarea, { target: { value: '[3, 2, 1]' } }); + expect(textarea.value).toBe('[3, 2, 1]'); + fireEvent.click(getByText(messages.buttonCancelText.defaultMessage)); + expect(textarea.value).toBe('[]'); }); it('should update the textarea value and display the updated value after clicking "Change manually"', async () => { const { getByLabelText, getByText } = render(); + let textarea; await waitFor(() => { - const textarea = getByLabelText(/Advanced Module List/i); - fireEvent.change(textarea, { target: { value: '[3, 2, 1' } }); - fireEvent.click(getByText(messages.buttonSaveText.defaultMessage)); - fireEvent.click(getByText(/Change manually/i)); - expect(textarea.value).toBe('[3, 2, 1'); + textarea = getByLabelText(/Advanced Module List/i); + }); + fireEvent.change(textarea, { target: { value: '[3, 2, 1,' } }); + expect(textarea.value).toBe('[3, 2, 1,'); + fireEvent.click(getByText('Save changes')); + fireEvent.click(getByText('Change manually')); + expect(textarea.value).toBe('[3, 2, 1,'); + }); + it('should show success alert after save', async () => { + const { getByLabelText, getByText } = render(); + let textarea; + await waitFor(() => { + textarea = getByLabelText(/Advanced Module List/i); }); + fireEvent.change(textarea, { target: { value: '[3, 2, 1]' } }); + expect(textarea.value).toBe('[3, 2, 1]'); + axiosMock + .onPatch(`${getCourseAdvancedSettingsApiUrl(courseId)}`) + .reply(200, { + ...advancedSettingsMock, + advancedModules: { + ...advancedSettingsMock.advancedModules, + value: [3, 2, 1], + }, + }); + fireEvent.click(getByText('Save changes')); + await executeThunk(updateCourseAppSetting(courseId, [3, 2, 1]), store.dispatch); + expect(getByText('Your policy changes have been saved.')).toBeInTheDocument(); }); }); diff --git a/src/advanced-settings/__mocks__/advancedSettings.js b/src/advanced-settings/__mocks__/advancedSettings.js index 52d5bed6b0..ead6c3528b 100644 --- a/src/advanced-settings/__mocks__/advancedSettings.js +++ b/src/advanced-settings/__mocks__/advancedSettings.js @@ -6,4 +6,11 @@ module.exports = { hideOnEnabledPublisher: false, value: [], }, + certHtmlViewEnabled: { + deprecated: true, + display_name: 'Certificate web/html view enabled', + help: 'If true, certificate Web/HTML views are enabled for the course.', + hide_on_enabled_publisher: false, + value: true, + }, }; diff --git a/src/advanced-settings/data/thunks.js b/src/advanced-settings/data/thunks.js index 99a421ffd8..db9030fa39 100644 --- a/src/advanced-settings/data/thunks.js +++ b/src/advanced-settings/data/thunks.js @@ -19,7 +19,20 @@ export function fetchCourseAppSettings(courseId) { try { const settingValues = await getCourseAdvancedSettings(courseId); - dispatch(fetchCourseAppsSettingsSuccess(settingValues)); + const sortedDisplayName = []; + Object.values(settingValues).forEach(value => { + const { displayName } = value; + sortedDisplayName.push(displayName); + }); + const sortedSettingValues = {}; + sortedDisplayName.sort().forEach((displayName => { + Object.entries(settingValues).forEach(([key, value]) => { + if (value.displayName === displayName) { + sortedSettingValues[key] = value; + } + }); + })); + dispatch(fetchCourseAppsSettingsSuccess(sortedSettingValues)); dispatch(updateLoadingStatus({ status: RequestStatus.SUCCESSFUL })); } catch (error) { if (error.response && error.response.status === 403) { diff --git a/src/advanced-settings/scss/AdvancedSettings.scss b/src/advanced-settings/scss/AdvancedSettings.scss index 3922addf1a..676c6dc81c 100644 --- a/src/advanced-settings/scss/AdvancedSettings.scss +++ b/src/advanced-settings/scss/AdvancedSettings.scss @@ -8,8 +8,8 @@ .instructions, strong { - font: normal $font-weight-normal .875rem/1.5rem $font-family-base; color: $text-color-base; + font-weight: 400; } } @@ -18,11 +18,6 @@ .pgn__card-header .pgn__card-header-title-md { font-size: 1.125rem; - padding-top: .625rem; - } - - .pgn__icon { - color: $headings-color; } } @@ -44,7 +39,6 @@ } .form-control { - resize: none; min-height: 2.75rem; width: $setting-form-control-width; } @@ -54,7 +48,8 @@ } .pgn__card-header { - padding: 0 1.5rem; + padding: 0 0 0 1.5rem; + flex-grow: 1; } .pgn__card-status { @@ -66,12 +61,39 @@ margin-bottom: 1.438rem; display: flex; flex-direction: revert; + } +} + +.setting-sidebar-supplementary { + .setting-sidebar-supplementary-about { + .setting-sidebar-supplementary-about-title { + font: normal $font-weight-bold 1.125rem/1.5rem $font-family-base; + color: $headings-color; + margin-bottom: 1.25rem; + } + + .setting-sidebar-supplementary-about-descriptions { + font: normal $font-weight-normal .875rem/1.5rem $font-family-base; + color: $text-color-base; + } + } + + .setting-sidebar-supplementary-other-links ul { + list-style: none; - .pgn__card-header-subtitle-md { - margin-left: .375rem; - margin-top: .125rem; + .setting-sidebar-supplementary-other-link { + font: normal $font-weight-normal .875rem/1.5rem $font-family-base; + line-height: 1.5rem; + color: $info-500; + margin-bottom: .5rem; } } + + .setting-sidebar-supplementary-other-title { + font: normal $font-weight-bold 1.125rem/1.5rem $font-family-base; + color: $headings-color; + margin-bottom: 1.25rem; + } } .modal-error-item { @@ -89,3 +111,15 @@ align-items: center; } } + +.modal-popup-content { + max-width: 200px; + color: $white; + background-color: $black; + filter: drop-shadow(0 2px 4px rgba(0 0 0 / .15)); + font-weight: 400; +} + +.pgn__modal-popup__arrow::after { + border-top-color: $black; +} diff --git a/src/advanced-settings/setting-card/SettingCard.jsx b/src/advanced-settings/setting-card/SettingCard.jsx index b4ff22aad8..15cef8ad9f 100644 --- a/src/advanced-settings/setting-card/SettingCard.jsx +++ b/src/advanced-settings/setting-card/SettingCard.jsx @@ -1,10 +1,15 @@ -import React from 'react'; +import React, { useState } from 'react'; import { - Card, Form, Icon, IconButton, OverlayTrigger, Popover, + ActionRow, + Card, + Form, + Icon, + IconButton, + ModalPopup, + useToggle, } from '@edx/paragon'; -import { Info, Warning } from '@edx/paragon/icons'; +import { InfoOutline, Warning } from '@edx/paragon/icons'; import PropTypes from 'prop-types'; -import classNames from 'classnames'; import { capitalize } from 'lodash'; import { injectIntl, intlShape } from '@edx/frontend-platform/i18n'; import TextareaAutosize from 'react-textarea-autosize'; @@ -12,47 +17,87 @@ import TextareaAutosize from 'react-textarea-autosize'; import messages from './messages'; const SettingCard = ({ - intl, showDeprecated, name, onChange, value, settingData, handleBlur, + name, + settingData, + handleBlur, + setEdited, + showSaveSettingsPrompt, + saveSettingsPrompt, + isEditableState, + setIsEditableState, + // injected + intl, }) => { const { deprecated, help, displayName } = settingData; + const initialValue = JSON.stringify(settingData.value, null, 4); + const [isOpen, open, close] = useToggle(false); + const [target, setTarget] = useState(null); + const [newValue, setNewValue] = useState(initialValue); + + const handleSettingChange = (e) => { + const { value } = e.target; + setNewValue(e.target.value); + if (value !== initialValue) { + if (!saveSettingsPrompt) { + showSaveSettingsPrompt(true); + } + if (!isEditableState) { + setIsEditableState(true); + } + } + }; + + const handleCardBlur = () => { + setEdited((prevEditedSettings) => ({ + ...prevEditedSettings, + [name]: newValue, + })); + handleBlur(); + }; + return ( -
  • +
  • - + - - {/* eslint-disable-next-line react/no-danger */} -
    - - - )} - > + title={( + + {capitalize(displayName)} - + +
    + + )} /> @@ -73,22 +118,21 @@ SettingCard.propTypes = { deprecated: PropTypes.bool, help: PropTypes.string, displayName: PropTypes.string, + value: PropTypes.PropTypes.oneOfType([ + PropTypes.string, + PropTypes.bool, + PropTypes.number, + PropTypes.object, + PropTypes.array, + ]), }).isRequired, - value: PropTypes.oneOfType([ - PropTypes.string, - PropTypes.bool, - PropTypes.number, - PropTypes.object, - PropTypes.array, - ]), - onChange: PropTypes.func.isRequired, - showDeprecated: PropTypes.bool.isRequired, + setEdited: PropTypes.func.isRequired, + showSaveSettingsPrompt: PropTypes.func.isRequired, name: PropTypes.string.isRequired, handleBlur: PropTypes.func.isRequired, -}; - -SettingCard.defaultProps = { - value: undefined, + saveSettingsPrompt: PropTypes.bool.isRequired, + isEditableState: PropTypes.bool.isRequired, + setIsEditableState: PropTypes.func.isRequired, }; export default injectIntl(SettingCard); diff --git a/src/advanced-settings/setting-card/SettingCard.test.jsx b/src/advanced-settings/setting-card/SettingCard.test.jsx index cb3aef74a3..a03f51a985 100644 --- a/src/advanced-settings/setting-card/SettingCard.test.jsx +++ b/src/advanced-settings/setting-card/SettingCard.test.jsx @@ -1,16 +1,21 @@ import React from 'react'; -import { render } from '@testing-library/react'; +import { fireEvent, render, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; import { IntlProvider } from '@edx/frontend-platform/i18n'; import SettingCard from './SettingCard'; import messages from './messages'; -const handleChange = jest.fn(); +const setEdited = jest.fn(); +const showSaveSettingsPrompt = jest.fn(); +const setIsEditableState = jest.fn(); +const handleBlur = jest.fn(); const settingData = { deprecated: false, help: 'This is a help message', displayName: 'Setting Name', + value: 'Setting Value', }; jest.mock('react-textarea-autosize', () => jest.fn((props) => ( @@ -27,9 +32,13 @@ const RootWrapper = () => ( intl={{}} isOn name="settingName" - onChange={handleChange} - value="Setting Value" + setEdited={setEdited} + setIsEditableState={setIsEditableState} + showSaveSettingsPrompt={showSaveSettingsPrompt} settingData={settingData} + handleBlur={handleBlur} + isEditableState + saveSettingsPrompt={false} /> ); @@ -42,7 +51,7 @@ describe('', () => { const input = getByLabelText(/Setting Name/i); expect(cardTitle).toBeInTheDocument(); expect(input).toBeInTheDocument(); - expect(input.value).toBe('Setting Value'); + expect(input.value).toBe(JSON.stringify(settingData.value, null, 4)); }); it('displays the deprecated status when the setting is deprecated', () => { const deprecatedSettingData = { ...settingData, deprecated: true }; @@ -52,9 +61,13 @@ describe('', () => { intl={{}} isOn name="settingName" - onChange={handleChange} - value="Setting Value" + setEdited={setEdited} + setIsEditableState={setIsEditableState} + showSaveSettingsPrompt={showSaveSettingsPrompt} settingData={deprecatedSettingData} + handleBlur={handleBlur} + isEditable={false} + saveSettingsPrompt /> , ); @@ -65,4 +78,19 @@ describe('', () => { const { queryByText } = render(); expect(queryByText(messages.deprecated.defaultMessage)).toBeNull(); }); + it('calls setEdited on blur', async () => { + const { getByLabelText } = render(); + const inputBox = getByLabelText(/Setting Name/i); + fireEvent.focus(inputBox); + userEvent.clear(inputBox); + userEvent.type(inputBox, '3, 2, 1'); + await waitFor(() => { + expect(inputBox).toHaveValue('3, 2, 1'); + }); + await (async () => { + expect(setEdited).toHaveBeenCalled(); + expect(handleBlur).toHaveBeenCalled(); + }); + fireEvent.focusOut(inputBox); + }); }); diff --git a/src/custom-pages/CustomPages.jsx b/src/custom-pages/CustomPages.jsx index 8b82cf0d0e..b7019ba10e 100644 --- a/src/custom-pages/CustomPages.jsx +++ b/src/custom-pages/CustomPages.jsx @@ -16,6 +16,7 @@ import { useToggle, Image, ModalDialog, + Container, } from '@edx/paragon'; import { Add, SpinnerSimple } from '@edx/paragon/icons'; import Placeholder, { @@ -110,7 +111,7 @@ const CustomPages = ({ } return ( -
    +
    -
    +     ); };