Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FC-0036] feat: Change behaviour of Tag Drawer on click outside #965

Merged
merged 4 commits into from
May 8, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/content-tags-drawer/ContentTagsDrawer.jsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// @ts-check
import React, { useEffect } from 'react';
import React, { useContext, useEffect } from 'react';
import PropTypes from 'prop-types';
import {
Container,
Expand All @@ -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.
Expand All @@ -32,6 +32,7 @@ const ContentTagsDrawer = ({ id, onClose }) => {
const contentId = id ?? params.contentId;

const context = useContentTagsDrawerContext(contentId);
const { blockingSheet } = useContext(ContentTagsDrawerSheetContext);

const {
showToastAfterSave,
Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's simpler to do this instead:

const blockingSheet = (Object.keys(globalStagedContentTags).length + Object.keys(globalStagedRemovedContentTags).length) > 0;

There doesn't seem to be any need to add another context, and state variable for something that can already be derived from existing info.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is better to leave it as context since it is used in two different places:

I prefer it this way, because in the first case it is not possible to calculate it there, and it is better to calculate that value in one place.

I think it's simpler to do this instead:
const blockingSheet = (Object.keys(globalStagedContentTags).length + Object.keys(globalStagedRemovedContentTags).length) > 0;

Furthermore, it could not be calculated in this way, because there is a case in which there are keys in the map, but they contain empty lists (add a tag, and remove the same tag later).

onCloseDrawer();
}
};
Expand All @@ -73,7 +74,7 @@ const ContentTagsDrawer = ({ id, onClose }) => {
return () => {
document.removeEventListener('keydown', handleEsc);
};
}, []);
}, [blockingSheet]);

useEffect(() => {
/* istanbul ignore next */
Expand Down
64 changes: 59 additions & 5 deletions src/content-tags-drawer/ContentTagsDrawer.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ 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();
Expand Down Expand Up @@ -61,15 +62,18 @@ jest.mock('../taxonomy/data/api', () => ({
const queryClient = new QueryClient();

const RootWrapper = (params) => (
<IntlProvider locale="en" messages={{}}>
<QueryClientProvider client={queryClient}>
<ContentTagsDrawer {...params} />
</QueryClientProvider>
</IntlProvider>
<ContentTagsDrawerSheetContext.Provider value={params}>
<IntlProvider locale="en" messages={{}}>
<QueryClientProvider client={queryClient}>
<ContentTagsDrawer {...params} />
</QueryClientProvider>
</IntlProvider>
</ContentTagsDrawerSheetContext.Provider>
);

describe('<ContentTagsDrawer />', () => {
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.
Expand Down Expand Up @@ -749,6 +753,16 @@ describe('<ContentTagsDrawer />', () => {
postMessageSpy.mockRestore();
});

it('should call `onClose` when Escape key is pressed and no selectable box is active', () => {
const { container } = render(<RootWrapper onClose={mockOnClose} />);

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');

Expand All @@ -771,6 +785,46 @@ describe('<ContentTagsDrawer />', () => {
postMessageSpy.mockRestore();
});

it('should not call `onClose` when Escape key is pressed and a selectable box is active', () => {
const { container } = render(<RootWrapper onClose={mockOnClose} />);

// 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(<RootWrapper blockingSheet />);
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(<RootWrapper blockingSheet onClose={mockOnClose} />);
fireEvent.keyDown(container, {
key: 'Escape',
});

expect(mockOnClose).not.toHaveBeenCalled();
});

it('should call `updateTags` mutation on save', async () => {
setupMockDataForStagedTagsTesting();
render(<RootWrapper />);
Expand Down
32 changes: 28 additions & 4 deletions src/content-tags-drawer/ContentTagsDrawerHelper.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
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 */
Expand Down Expand Up @@ -48,6 +49,8 @@
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.
Expand Down Expand Up @@ -235,20 +238,32 @@
setCollapsibleToInitalState,
]);

// Build toast message and show toast after save drawer.
/* istanbul ignore next */
const showToastAfterSave = React.useCallback(() => {
// Count added and removed tags
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(
Expand All @@ -273,7 +288,7 @@
);
}
setToastMessage(message);
}, [globalStagedContentTags, globalStagedRemovedContentTags, setToastMessage]);
}, [setToastMessage, countTags]);

// Close the toast
const closeToast = React.useCallback(() => setToastMessage(undefined), [setToastMessage]);
Expand Down Expand Up @@ -321,6 +336,15 @@
const mergedTagsArray = fechedTaxonomies.map(obj => mergedTags[obj.id]);

setTagsByTaxonomy(mergedTagsArray);

if (setBlockingSheet) {
const { tagsAdded, tagsRemoved } = countTags();
if (tagsAdded || tagsRemoved) {
setBlockingSheet(true);

Check warning on line 343 in src/content-tags-drawer/ContentTagsDrawerHelper.jsx

View check run for this annotation

Codecov / codecov/patch

src/content-tags-drawer/ContentTagsDrawerHelper.jsx#L343

Added line #L343 was not covered by tests
} else {
setBlockingSheet(false);
}
}
}, [
fechedTaxonomies,
globalStagedContentTags,
Expand Down
44 changes: 44 additions & 0 deletions src/content-tags-drawer/ContentTagsDrawerSheet.jsx
Original file line number Diff line number Diff line change
@@ -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 (
<ContentTagsDrawerSheetContext.Provider value={context}>
<Sheet
position="right"
show={showSheet}
onClose={onClose}
blocking={blockingSheet}
>
<ContentTagsDrawer
id={id}
onClose={onClose}
/>
</Sheet>
</ContentTagsDrawerSheetContext.Provider>
);
};

ContentTagsDrawerSheet.propTypes = {
id: PropTypes.string,
onClose: PropTypes.func,
showSheet: PropTypes.bool,
};

ContentTagsDrawerSheet.defaultProps = {
id: undefined,
onClose: undefined,
showSheet: false,
};

export default ContentTagsDrawerSheet;
6 changes: 6 additions & 0 deletions src/content-tags-drawer/common/context.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,9 @@ export const ContentTagsDrawerContext = React.createContext({
closeToast: /** @type{() => void} */ (() => {}),
setCollapsibleToInitalState: /** @type{() => void} */ (() => {}),
});

/* istanbul ignore next */
export const ContentTagsDrawerSheetContext = React.createContext({
blockingSheet: /** @type{boolean} */ (false),
setBlockingSheet: /** @type{Function} */ (() => {}),
});
Comment on lines +44 to +48
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason to add another context instead of using the same one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, it seems like the value of 'blockingSheet' is just derived from whether tags have been added or not, an actual count is not important.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added one more layer above, currently it is like this: ContentTagsDrawerSheet > ContentTagsDrawer > ContentTagsCollapsible. The drawer is used in:

  • frontend-app-course-authoring UI: Here ContentTagsDrawerSheet is used.
  • edx-platform UI as iframe: Here ContentTagsDrawer is used, not ContentTagsDrawerSheet.

I have separated it because ContentTagsDrawerSheetContext is ContentTagsDrawerSheet logic and is not used in the legacy edx-platform screens. This will change in the future, so I'm going to add comments in the code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, it seems like the value of 'blockingSheet' is just derived from whether tags have been added or not, an actual count is not important.

Updated here: 1fe6282

Copy link
Contributor Author

@ChrisChV ChrisChV Apr 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have separated it because ContentTagsDrawerSheetContext is ContentTagsDrawerSheet logic and is not used in the legacy edx-platform screens. This will change in the future, so I'm going to add comments in the code

Comments added here: cf746b4

2 changes: 2 additions & 0 deletions src/content-tags-drawer/index.js
Original file line number Diff line number Diff line change
@@ -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';
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -93,16 +93,11 @@ const TagsSidebarBody = () => {
</Button>
</Stack>
</Card.Body>
<Sheet
position="right"
show={showManageTags}
<ContentTagsDrawerSheet
id={contentId}
onClose={onClose}
>
<ContentTagsDrawer
id={contentId}
onClose={onClose}
/>
</Sheet>
showSheet={showManageTags}
/>
</>
);
};
Expand Down
16 changes: 5 additions & 11 deletions src/course-outline/card-header/CardHeader.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import {
Hyperlink,
Icon,
IconButton,
Sheet,
useToggle,
} from '@openedx/paragon';
import {
Expand All @@ -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';
Expand Down Expand Up @@ -229,16 +228,11 @@ const CardHeader = ({
</Dropdown>
</div>
</div>
<Sheet
position="right"
show={isManageTagsDrawerOpen}
<ContentTagsDrawerSheet
id={cardId}
onClose={/* istanbul ignore next */ () => closeManageTagsDrawer()}
>
<ContentTagsDrawer
id={cardId}
onClose={/* istanbul ignore next */ () => closeManageTagsDrawer()}
/>
</Sheet>
showSheet={isManageTagsDrawerOpen}
/>
</>
);
};
Expand Down
17 changes: 6 additions & 11 deletions src/course-outline/status-bar/StatusBar.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -188,16 +188,11 @@ const StatusBar = ({

)}
</Stack>
<Sheet
position="right"
show={isManageTagsDrawerOpen}
<ContentTagsDrawerSheet
id={courseId}
onClose={/* istanbul ignore next */ () => closeManageTagsDrawer()}
>
<ContentTagsDrawer
id={courseId}
onClose={/* istanbul ignore next */ () => closeManageTagsDrawer()}
/>
</Sheet>
showSheet={isManageTagsDrawerOpen}
/>
</>
);
};
Expand Down
Loading