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

ChrisChV · 2024-04-25T20:01:28Z

Description

https://www.loom.com/share/c87289b025b34b9ab2381a749dd9db4f?sid=fffede42-3e4d-4cf9-9dae-c0a72a7c2397

Block close on click ouside an on press Escape to avoid lose user data.
Wrap with ContentTagsDrawerSheet to build drawer on MFE

Supporting information

Github issue: [Tagging] Change behaviour of Tag Drawer on click outside modular-learning#209
This PR doesn't include:

The footer of the drawer animates to draw the user's attention to the actions (see Figma). Perhaps like Blink 2 at this link

Internal ticket: FAL-3711
Depends on: [FC-0036] Refined tag drawer #939

Testing instructions

Use this branch of edx-platform: Bump openedx-learning to support tagging with multiple taxonomies at once [FC-0036] edx-platform#34490
Run make requirements on cms shell.
Make sure you have some sample taxonomy/tags data setup: https://github.com/open-craft/taxonomy-sample-data
Go into a course and click on "Manage tags" option for any unit/block to open the tags drawer
Click on Edit tags
Delete a tags and verify that can't close drawer when clicking outside.
Cancel and verify that you can close drawer when clicking outside.
Click on Edit tags
Add new tags and verify that can't close drawer when clicking outside.
Press Escape and verify that can't close drawer

openedx-webhooks · 2024-04-25T20:01:32Z

Thanks for the pull request, @ChrisChV! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

codecov · 2024-04-25T20:10:52Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.08%. Comparing base (9327948) to head (05ebd9d).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #965      +/-   ##
==========================================
+ Coverage   92.06%   92.08%   +0.01%     
==========================================
  Files         685      686       +1     
  Lines       12054    12075      +21     
  Branches     2626     2631       +5     
==========================================
+ Hits        11098    11119      +21     
  Misses        920      920              
  Partials       36       36

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

* Avoid close Tags drawer on edit * To avoid lose user data * Wrap with ContentTagsDrawerSheet to build drawer on MFE * ContentTagsDrawerSheetContext created * Not close drawer with Escape is pressed when container is blocked

xitij2000

For me, this just isn't working. I can edit the tags, but escape is still working.

Additionally, I don't see any code to prevent it from closing when clicking outside.

I think this can be simplified a lot. I've given example code that I tested, and it's working. All the context is already available there.

xitij2000 · 2024-04-26T05:50:52Z

src/content-tags-drawer/common/context.js

+/* istanbul ignore next */
+export const ContentTagsDrawerSheetContext = React.createContext({
+  blockingSheet: /** @type{boolean} */ (false),
+  setBlockingSheet: /** @type{Function} */ (() => {}),
+});


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

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.

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

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

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

xitij2000 · 2024-04-26T08:21:00Z

src/content-tags-drawer/ContentTagsDrawer.jsx

@@ -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) {


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.

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

To avoid close on click outside: https://github.com/openedx/frontend-app-course-authoring/pull/965/files#diff-44e5b0a241ffd4d6230ede25608e5392858869b1b6761b64f1daac7c18605ff2R21

To avoid close on press Escape: https://github.com/openedx/frontend-app-course-authoring/pull/965/files#diff-8f2b1f4e041cee96dfc16c4c3da7631943407c989c2ea619594681f0c40e775bR68

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).

ChrisChV · 2024-04-26T16:50:28Z

Additionally, I don't see any code to prevent it from closing when clicking outside.

This functionality exists in paragon's Sheet component. I have added it there using blockingSheet logic.

For me, this just isn't working. I can edit the tags, but escape is still working.

I think it is a confusion, the requirement is: If the user clicks on the grey area outside of the tagging drawer AND there are unsaved changes (at least one added/removed tag). So it only blocks if the user has added or removed tags. Otherwise it doesn't block:

https://www.loom.com/share/c87289b025b34b9ab2381a749dd9db4f?sid=fffede42-3e4d-4cf9-9dae-c0a72a7c2397

ChrisChV · 2024-04-26T16:51:51Z

@xitij2000 It's ready for another review

xitij2000

OK. I think the issue I had was that I was testing with the old studio, however this seems to be implemented only for the new UI?

If this is supposed to work on the current studio as well, then this needs to be fixed. Otherwise I can merge.

ChrisChV · 2024-05-07T15:49:17Z

If this is supposed to work on the current studio as well, then this needs to be fixed. Otherwise I can merge.

I think it would take more time (ref). But, what do you think @bradenmacdonald? I can do it in another PR

bradenmacdonald · 2024-05-07T17:40:28Z

I'll ask about it. But in the meantime, please merge this, and if we need to implement it in the legacy UI, we'll do it in another PR.

openedx-webhooks · 2024-05-08T19:56:35Z

@ChrisChV 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

ormsbee · 2024-05-08T19:57:54Z

I played merge monkey on this one since this already had @xitij2000's approval, but he's out sick.

ChrisChV requested a review from a team as a code owner April 25, 2024 20:01

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Apr 25, 2024

ChrisChV marked this pull request as draft April 25, 2024 20:01

ChrisChV force-pushed the chris/FAL-3711-close-drawer branch from d36bf33 to 73c978b Compare April 25, 2024 20:05

ChrisChV mentioned this pull request Apr 25, 2024

feat: Avoid close Tags drawer on edit open-craft/frontend-app-authoring#34

Closed

ChrisChV marked this pull request as ready for review April 25, 2024 20:09

ChrisChV changed the title ~~feat: Change behaviour of Tag Drawer on click outside~~ [FC-0036] feat: Change behaviour of Tag Drawer on click outside Apr 25, 2024

ChrisChV force-pushed the chris/FAL-3711-close-drawer branch 2 times, most recently from 9004ea8 to 30f19c7 Compare April 25, 2024 20:46

feat: Change behaviour of Tag Drawer on click outside

9bfe994

* Avoid close Tags drawer on edit * To avoid lose user data * Wrap with ContentTagsDrawerSheet to build drawer on MFE * ContentTagsDrawerSheetContext created * Not close drawer with Escape is pressed when container is blocked

ChrisChV force-pushed the chris/FAL-3711-close-drawer branch from 30f19c7 to 9bfe994 Compare April 25, 2024 21:33

xitij2000 requested changes Apr 26, 2024

View reviewed changes

ChrisChV added 2 commits April 26, 2024 11:30

refactor: Function to check BlockingSheet value

1fe6282

docs: Comments on the code

cf746b4

chore: leave countTags out of coverage

05ebd9d

xitij2000 approved these changes May 7, 2024

View reviewed changes

ormsbee merged commit dd9202f into openedx:master May 8, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ChrisChV commented Apr 25, 2024 •

edited

Loading

openedx-webhooks commented Apr 25, 2024 •

edited

Loading

codecov bot commented Apr 25, 2024 •

edited

Loading

xitij2000 left a comment

xitij2000 Apr 26, 2024

xitij2000 Apr 26, 2024

ChrisChV Apr 26, 2024

ChrisChV Apr 26, 2024

ChrisChV Apr 26, 2024 •

edited

Loading

xitij2000 Apr 26, 2024

ChrisChV Apr 26, 2024

ChrisChV commented Apr 26, 2024

ChrisChV commented Apr 26, 2024

xitij2000 left a comment

ChrisChV commented May 7, 2024

bradenmacdonald commented May 7, 2024

openedx-webhooks commented May 8, 2024

ormsbee commented May 8, 2024

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

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

Conversation

ChrisChV commented Apr 25, 2024 • edited Loading

Description

Supporting information

Testing instructions

openedx-webhooks commented Apr 25, 2024 • edited Loading

codecov bot commented Apr 25, 2024 • edited Loading

Codecov Report

xitij2000 left a comment

Choose a reason for hiding this comment

xitij2000 Apr 26, 2024

Choose a reason for hiding this comment

xitij2000 Apr 26, 2024

Choose a reason for hiding this comment

ChrisChV Apr 26, 2024

Choose a reason for hiding this comment

ChrisChV Apr 26, 2024

Choose a reason for hiding this comment

ChrisChV Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

xitij2000 Apr 26, 2024

Choose a reason for hiding this comment

ChrisChV Apr 26, 2024

Choose a reason for hiding this comment

ChrisChV commented Apr 26, 2024

ChrisChV commented Apr 26, 2024

xitij2000 left a comment

Choose a reason for hiding this comment

ChrisChV commented May 7, 2024

bradenmacdonald commented May 7, 2024

openedx-webhooks commented May 8, 2024

ormsbee commented May 8, 2024

ChrisChV commented Apr 25, 2024 •

edited

Loading

openedx-webhooks commented Apr 25, 2024 •

edited

Loading

codecov bot commented Apr 25, 2024 •

edited

Loading

ChrisChV Apr 26, 2024 •

edited

Loading