-
Notifications
You must be signed in to change notification settings - Fork 210
[FC-0036] feat: Change behaviour of Tag Drawer on click outside #965
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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} */ (() => {}), | ||
| }); | ||
|
Comment on lines
+44
to
+48
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have added one more layer above, currently it is like this:
I have separated it because
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Updated here: 1fe6282
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Comments added here: cf746b4 |
||
| 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'; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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).