From dd9202fafef67a78fa16ea4d1bb6bad46f48a449 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Chris=20Ch=C3=A1vez?= Date: Wed, 8 May 2024 14:56:32 -0500 Subject: [PATCH] feat: prevent losing work when users click outside tag drawer after making edits [FC-0036] --- src/content-tags-drawer/ContentTagsDrawer.jsx | 9 +- .../ContentTagsDrawer.test.jsx | 117 +++++++++++++++++- .../ContentTagsDrawerHelper.jsx | 47 ++++++- .../ContentTagsDrawerSheet.jsx | 44 +++++++ src/content-tags-drawer/common/context.js | 11 ++ src/content-tags-drawer/index.js | 2 + .../tags-sidebar-controls/TagsSidebarBody.jsx | 17 +-- src/course-outline/card-header/CardHeader.jsx | 16 +-- src/course-outline/status-bar/StatusBar.jsx | 17 +-- 9 files changed, 235 insertions(+), 45 deletions(-) create mode 100644 src/content-tags-drawer/ContentTagsDrawerSheet.jsx diff --git a/src/content-tags-drawer/ContentTagsDrawer.jsx b/src/content-tags-drawer/ContentTagsDrawer.jsx index b26d87d177..d8a69079f4 100644 --- a/src/content-tags-drawer/ContentTagsDrawer.jsx +++ b/src/content-tags-drawer/ContentTagsDrawer.jsx @@ -1,5 +1,5 @@ // @ts-check -import React, { useEffect } from 'react'; +import React, { useContext, useEffect } from 'react'; import PropTypes from 'prop-types'; import { Container, @@ -14,7 +14,7 @@ import messages from './messages'; import ContentTagsCollapsible from './ContentTagsCollapsible'; import Loading from '../generic/Loading'; import useContentTagsDrawerContext from './ContentTagsDrawerHelper'; -import { ContentTagsDrawerContext } from './common/context'; +import { ContentTagsDrawerContext, ContentTagsDrawerSheetContext } from './common/context'; /** * Drawer with the functionality to show and manage tags in a certain content. @@ -32,6 +32,7 @@ const ContentTagsDrawer = ({ id, onClose }) => { const contentId = id ?? params.contentId; const context = useContentTagsDrawerContext(contentId); + const { blockingSheet } = useContext(ContentTagsDrawerSheetContext); const { showToastAfterSave, @@ -64,7 +65,7 @@ const ContentTagsDrawer = ({ id, onClose }) => { const handleEsc = (event) => { /* Close drawer when ESC-key is pressed and selectable dropdown box not open */ const selectableBoxOpen = document.querySelector('[data-selectable-box="taxonomy-tags"]'); - if (event.key === 'Escape' && !selectableBoxOpen) { + if (event.key === 'Escape' && !selectableBoxOpen && !blockingSheet) { onCloseDrawer(); } }; @@ -73,7 +74,7 @@ const ContentTagsDrawer = ({ id, onClose }) => { return () => { document.removeEventListener('keydown', handleEsc); }; - }, []); + }, [blockingSheet]); useEffect(() => { /* istanbul ignore next */ diff --git a/src/content-tags-drawer/ContentTagsDrawer.test.jsx b/src/content-tags-drawer/ContentTagsDrawer.test.jsx index 03de825cc6..19376ac673 100644 --- a/src/content-tags-drawer/ContentTagsDrawer.test.jsx +++ b/src/content-tags-drawer/ContentTagsDrawer.test.jsx @@ -19,10 +19,12 @@ import { } from './data/apiHooks'; import { getTaxonomyListData } from '../taxonomy/data/api'; import messages from './messages'; +import { ContentTagsDrawerSheetContext } from './common/context'; const contentId = 'block-v1:SampleTaxonomyOrg1+STC1+2023_1+type@vertical+block@7f47fe2dbcaf47c5a071671c741fe1ab'; const mockOnClose = jest.fn(); const mockMutate = jest.fn(); +const mockSetBlockingSheet = jest.fn(); jest.mock('react-router-dom', () => ({ ...jest.requireActual('react-router-dom'), @@ -61,15 +63,18 @@ jest.mock('../taxonomy/data/api', () => ({ const queryClient = new QueryClient(); const RootWrapper = (params) => ( - - - - - + + + + + + + ); describe('', () => { beforeEach(async () => { + jest.clearAllMocks(); await queryClient.resetQueries(); // By default, we mock the API call with a promise that never resolves. // You can override this in specific test. @@ -749,6 +754,16 @@ describe('', () => { postMessageSpy.mockRestore(); }); + it('should call `onClose` when Escape key is pressed and no selectable box is active', () => { + const { container } = render(); + + fireEvent.keyDown(container, { + key: 'Escape', + }); + + expect(mockOnClose).toHaveBeenCalled(); + }); + it('should not call closeManageTagsDrawer when Escape key is pressed and a selectable box is active', () => { const postMessageSpy = jest.spyOn(window.parent, 'postMessage'); @@ -771,6 +786,98 @@ describe('', () => { postMessageSpy.mockRestore(); }); + it('should not call `onClose` when Escape key is pressed and a selectable box is active', () => { + const { container } = render(); + + // Simulate that the selectable box is open by adding an element with the data attribute + const selectableBox = document.createElement('div'); + selectableBox.setAttribute('data-selectable-box', 'taxonomy-tags'); + document.body.appendChild(selectableBox); + + fireEvent.keyDown(container, { + key: 'Escape', + }); + + expect(mockOnClose).not.toHaveBeenCalled(); + + // Remove the added element + document.body.removeChild(selectableBox); + }); + + it('should not call closeManageTagsDrawer when Escape key is pressed and container is blocked', () => { + const postMessageSpy = jest.spyOn(window.parent, 'postMessage'); + + const { container } = render(); + fireEvent.keyDown(container, { + key: 'Escape', + }); + + expect(postMessageSpy).not.toHaveBeenCalled(); + + postMessageSpy.mockRestore(); + }); + + it('should not call `onClose` when Escape key is pressed and container is blocked', () => { + const { container } = render(); + fireEvent.keyDown(container, { + key: 'Escape', + }); + + expect(mockOnClose).not.toHaveBeenCalled(); + }); + + it('should call `setBlockingSheet` on add a tag', async () => { + setupMockDataForStagedTagsTesting(); + render(); + expect(await screen.findByText('Taxonomy 1')).toBeInTheDocument(); + + expect(mockSetBlockingSheet).toHaveBeenCalledWith(false); + + // To edit mode + const editTagsButton = screen.getByRole('button', { + name: /edit tags/i, + }); + fireEvent.click(editTagsButton); + + // Click on "Add a tag" button to open dropdown + const addTagsButton = screen.getByText(/add a tag/i); + // Use `mouseDown` instead of `click` since the react-select didn't respond to `click` + fireEvent.mouseDown(addTagsButton); + + // Click to check Tag 3 + const tag3 = screen.getByText(/tag 3/i); + fireEvent.click(tag3); + + // Click "Add tags" to save to global staged tags + const addTags = screen.getByRole('button', { name: /add tags/i }); + fireEvent.click(addTags); + + expect(mockSetBlockingSheet).toHaveBeenCalledWith(true); + }); + + it('should call `setBlockingSheet` on delete a tag', async () => { + setupMockDataForStagedTagsTesting(); + render(); + expect(await screen.findByText('Taxonomy 1')).toBeInTheDocument(); + + expect(mockSetBlockingSheet).toHaveBeenCalledWith(false); + + // To edit mode + const editTagsButton = screen.getByRole('button', { + name: /edit tags/i, + }); + fireEvent.click(editTagsButton); + + // Delete the tag + const tag = screen.getByText(/tag 2/i); + const deleteButton = within(tag).getByRole('button', { + name: /delete/i, + }); + fireEvent.click(deleteButton); + + expect(mockSetBlockingSheet).toHaveBeenCalledWith(true); + }); + it('should call `updateTags` mutation on save', async () => { setupMockDataForStagedTagsTesting(); render(); diff --git a/src/content-tags-drawer/ContentTagsDrawerHelper.jsx b/src/content-tags-drawer/ContentTagsDrawerHelper.jsx index 3f6d5508c5..b74f9db4eb 100644 --- a/src/content-tags-drawer/ContentTagsDrawerHelper.jsx +++ b/src/content-tags-drawer/ContentTagsDrawerHelper.jsx @@ -6,6 +6,7 @@ import { useContentData, useContentTaxonomyTagsData, useContentTaxonomyTagsUpdat import { useTaxonomyList } from '../taxonomy/data/apiHooks'; import { extractOrgFromContentId } from './utils'; import messages from './messages'; +import { ContentTagsDrawerSheetContext } from './common/context'; /** @typedef {import("./data/types.mjs").Tag} ContentTagData */ /** @typedef {import("./data/types.mjs").StagedTagData} StagedTagData */ @@ -48,6 +49,8 @@ const useContentTagsDrawerContext = (contentId) => { const intl = useIntl(); const org = extractOrgFromContentId(contentId); + const { setBlockingSheet } = React.useContext(ContentTagsDrawerSheetContext); + // True if the drawer is on edit mode. const [isEditMode, setIsEditMode] = React.useState(false); // This stores the tags added on the add tags Select in all taxonomies. @@ -235,20 +238,33 @@ const useContentTagsDrawerContext = (contentId) => { setCollapsibleToInitalState, ]); - // Build toast message and show toast after save drawer. + // Count added and removed tags /* istanbul ignore next */ - const showToastAfterSave = React.useCallback(() => { + const countTags = React.useCallback(() => { const tagsAddedList = Object.values(globalStagedContentTags); const tagsRemovedList = Object.values(globalStagedRemovedContentTags); const tagsAdded = tagsAddedList.length === 1 ? tagsAddedList[0].length : tagsAddedList.reduce( + /* istanbul ignore next */ (acc, curr) => acc + curr.length, 0, ); const tagsRemoved = tagsRemovedList.length === 1 ? tagsRemovedList[0].length : tagsRemovedList.reduce( + /* istanbul ignore next */ (acc, curr) => acc + curr.length, 0, ); + return { + tagsAdded, + tagsRemoved, + }; + }, [globalStagedContentTags, globalStagedRemovedContentTags]); + + // Build toast message and show toast after save drawer. + /* istanbul ignore next */ + const showToastAfterSave = React.useCallback(() => { + const { tagsAdded, tagsRemoved } = countTags(); + let message; if (tagsAdded && tagsRemoved) { message = `${intl.formatMessage( @@ -273,7 +289,7 @@ const useContentTagsDrawerContext = (contentId) => { ); } setToastMessage(message); - }, [globalStagedContentTags, globalStagedRemovedContentTags, setToastMessage]); + }, [setToastMessage, countTags]); // Close the toast const closeToast = React.useCallback(() => setToastMessage(undefined), [setToastMessage]); @@ -321,6 +337,31 @@ const useContentTagsDrawerContext = (contentId) => { const mergedTagsArray = fechedTaxonomies.map(obj => mergedTags[obj.id]); setTagsByTaxonomy(mergedTagsArray); + + if (setBlockingSheet) { + const areChangesInTags = () => { + // It is calculated in this way, because there are cases in which + // there are keys in the map, but they contain empty lists + // (e.g. add a tag, and remove the same tag later). + + const tagsAddedList = Object.values(globalStagedContentTags); + const tagsRemovedList = Object.values(globalStagedRemovedContentTags); + + if (tagsAddedList.some(tags => tags.length > 0)) { + return true; + } + if (tagsRemovedList.some(tags => tags.length > 0)) { + return true; + } + return false; + }; + + if (areChangesInTags()) { + setBlockingSheet(true); + } else { + setBlockingSheet(false); + } + } }, [ fechedTaxonomies, globalStagedContentTags, diff --git a/src/content-tags-drawer/ContentTagsDrawerSheet.jsx b/src/content-tags-drawer/ContentTagsDrawerSheet.jsx new file mode 100644 index 0000000000..c37b7581f3 --- /dev/null +++ b/src/content-tags-drawer/ContentTagsDrawerSheet.jsx @@ -0,0 +1,44 @@ +// @ts-check +import React, { useMemo, useState } from 'react'; +import { Sheet } from '@openedx/paragon'; +import PropTypes from 'prop-types'; +import ContentTagsDrawer from './ContentTagsDrawer'; +import { ContentTagsDrawerSheetContext } from './common/context'; + +const ContentTagsDrawerSheet = ({ id, onClose, showSheet }) => { + const [blockingSheet, setBlockingSheet] = useState(false); + + const context = useMemo(() => ({ + blockingSheet, setBlockingSheet, + }), [blockingSheet, setBlockingSheet]); + + return ( + + + + + + ); +}; + +ContentTagsDrawerSheet.propTypes = { + id: PropTypes.string, + onClose: PropTypes.func, + showSheet: PropTypes.bool, +}; + +ContentTagsDrawerSheet.defaultProps = { + id: undefined, + onClose: undefined, + showSheet: false, +}; + +export default ContentTagsDrawerSheet; diff --git a/src/content-tags-drawer/common/context.js b/src/content-tags-drawer/common/context.js index 966e276e4b..875823200c 100644 --- a/src/content-tags-drawer/common/context.js +++ b/src/content-tags-drawer/common/context.js @@ -35,3 +35,14 @@ export const ContentTagsDrawerContext = React.createContext({ closeToast: /** @type{() => void} */ (() => {}), setCollapsibleToInitalState: /** @type{() => void} */ (() => {}), }); + +// This context has not been added to ContentTagsDrawerContext because it has been +// created one level higher to control the behavior of the Sheet that contatins the Drawer. +// This logic is not used in legacy edx-platform screens. But it can be separated if we keep +// the contexts separate. +// TODO We can join both contexts when the Drawer is no longer used on edx-platform +/* istanbul ignore next */ +export const ContentTagsDrawerSheetContext = React.createContext({ + blockingSheet: /** @type{boolean} */ (false), + setBlockingSheet: /** @type{Function} */ (() => {}), +}); diff --git a/src/content-tags-drawer/index.js b/src/content-tags-drawer/index.js index 8b9a48e354..87f64a4f6b 100644 --- a/src/content-tags-drawer/index.js +++ b/src/content-tags-drawer/index.js @@ -1,2 +1,4 @@ // eslint-disable-next-line import/prefer-default-export export { default as ContentTagsDrawer } from './ContentTagsDrawer'; +// eslint-disable-next-line import/prefer-default-export +export { default as ContentTagsDrawerSheet } from './ContentTagsDrawerSheet'; diff --git a/src/content-tags-drawer/tags-sidebar-controls/TagsSidebarBody.jsx b/src/content-tags-drawer/tags-sidebar-controls/TagsSidebarBody.jsx index d4ee05c588..ca7659a17f 100644 --- a/src/content-tags-drawer/tags-sidebar-controls/TagsSidebarBody.jsx +++ b/src/content-tags-drawer/tags-sidebar-controls/TagsSidebarBody.jsx @@ -1,12 +1,12 @@ // @ts-check import React, { useState, useMemo } from 'react'; import { - Card, Stack, Button, Sheet, Collapsible, Icon, + Card, Stack, Button, Collapsible, Icon, } from '@openedx/paragon'; import { ArrowDropDown, ArrowDropUp } from '@openedx/paragon/icons'; import { useIntl } from '@edx/frontend-platform/i18n'; import { useParams } from 'react-router-dom'; -import { ContentTagsDrawer } from '..'; +import { ContentTagsDrawerSheet } from '..'; import messages from '../messages'; import { useContentTaxonomyTagsData } from '../data/apiHooks'; @@ -93,16 +93,11 @@ const TagsSidebarBody = () => { - - - + showSheet={showManageTags} + /> ); }; diff --git a/src/course-outline/card-header/CardHeader.jsx b/src/course-outline/card-header/CardHeader.jsx index 3634e42e70..97ae602a45 100644 --- a/src/course-outline/card-header/CardHeader.jsx +++ b/src/course-outline/card-header/CardHeader.jsx @@ -10,7 +10,6 @@ import { Hyperlink, Icon, IconButton, - Sheet, useToggle, } from '@openedx/paragon'; import { @@ -19,7 +18,7 @@ import { } from '@openedx/paragon/icons'; import { useContentTagsCount } from '../../generic/data/apiHooks'; -import { ContentTagsDrawer } from '../../content-tags-drawer'; +import { ContentTagsDrawerSheet } from '../../content-tags-drawer'; import TagCount from '../../generic/tag-count'; import { useEscapeClick } from '../../hooks'; import { ITEM_BADGE_STATUS } from '../constants'; @@ -234,16 +233,11 @@ const CardHeader = ({ - closeManageTagsDrawer()} - > - closeManageTagsDrawer()} - /> - + showSheet={isManageTagsDrawerOpen} + /> ); }; diff --git a/src/course-outline/status-bar/StatusBar.jsx b/src/course-outline/status-bar/StatusBar.jsx index 97c4b93538..ed8fa28309 100644 --- a/src/course-outline/status-bar/StatusBar.jsx +++ b/src/course-outline/status-bar/StatusBar.jsx @@ -4,11 +4,11 @@ import PropTypes from 'prop-types'; import { FormattedDate, useIntl } from '@edx/frontend-platform/i18n'; import { getConfig } from '@edx/frontend-platform/config'; import { - Button, Hyperlink, Form, Sheet, Stack, useToggle, + Button, Hyperlink, Form, Stack, useToggle, } from '@openedx/paragon'; import { AppContext } from '@edx/frontend-platform/react'; -import { ContentTagsDrawer } from '../../content-tags-drawer'; +import { ContentTagsDrawerSheet } from '../../content-tags-drawer'; import TagCount from '../../generic/tag-count'; import { useHelpUrls } from '../../help-urls/hooks'; import { VIDEO_SHARING_OPTIONS } from '../constants'; @@ -188,16 +188,11 @@ const StatusBar = ({ )} - closeManageTagsDrawer()} - > - closeManageTagsDrawer()} - /> - + showSheet={isManageTagsDrawerOpen} + /> ); };