[FC-0036] Refined tag drawer #939

ChrisChV · 2024-04-05T22:44:25Z

Description

Multiple refinements to tag drawer: openedx/modular-learning#190

Read mode and edit mode of the drawer.
Global state to revert changes made in edit mode.
Save all changes on edit model when user clicks on Save.
New styles.

Supporting information

Github issue: [Tagging] Refined Tag Drawer modular-learning#190
Related PRs:
- Bump openedx-learning to support tagging with multiple taxonomies at once [FC-0036] edx-platform#34490
- [FC-0036] feat: Update ObjectTagView openedx-learning#180
Internal ticket: FAL-3645

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
Confirm:
- Taxonomies with tags added to them are open by default, and ordered according to the number of tags they contain
- Taxonomies with no tags are listed alphabetically below the taxonomies with tags
- A footer with actions has been added to the drawer. In read-only mode it displays "Close" and "Edit Tags".
- The new style of tags tree.
- You are on read only mode: You can't add/remove tags.
- You can close the drawer with the "Close" button
Click on "Edit tags" and confirm:
- X's appear next to deletable tags
- Any collapsed taxonomies expand
- "Add a tag" fields ([Tagging] New "Add Tags" Widget in Tag Drawer modular-learning#188) appear at the bottom of the sections for each taxonomy
- Buttons in the footer change to "cancel" and "save"
- Make sure the drawer scrolls
Add and delete tags for any taxonomy
Click on Cancel and verify that the drawer changes to read mode and all your changes are canceled.
Click on "Edit tags" and add/delete tags and click "Save" and verify.
- Your changes are saved.
- The drawer changes to Read-Only mode but stays open
- A toast appears in the bottom left of the page saying "N tags added", "N tags removed", or "X tags added, Y tags removed"
Test the different states of the toast:
- Add and remove tags.
- Only add tags.
- Only remove tags.

openedx-webhooks · 2024-04-05T22:44:29Z

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.

yusuf-musleh

@ChrisChV Great work on this! Overall its working quite well. Just found an issue and some small nits.

When checking a tag in edit mode and adding it in the edit mode, if you click "Cancel" it gets cleared however the checkbox stays checked. See video below for reproduction:

Screen.Recording.2024-04-11.at.3.59.42.PM.mov

The same also seems to happen when removing a tag in edit mode, the previously applied tags remains checked, see below:

Screen.Recording.2024-04-11.at.4.07.22.PM.mov

These 2 are likely related, it seems like it only happens when removing. It might be helpful to use the clear method:

const [checkedTags, { add, remove, clear }] = useCheckboxSetValues();

to clear all the checkboxes whenever tags are commit/saved and new ones are returned from the backend, and let the render handle checking the appropriate applied tags. However it might be something simpler that I am missing with the new code setup.

src/content-tags-drawer/ContentTagsCollapsibleHelper.jsx

src/content-tags-drawer/ContentTagsDrawerHelper.jsx

yusuf-musleh · 2024-04-11T11:59:52Z

src/content-tags-drawer/ContentTagsDrawerHelper.jsx

+      const taxonomiesWithData = taxonomiesList.filter(
+        (t) => t.contentTags.length !== 0,
+      ).sort((a, b) => b.contentTags.length - a.contentTags.length);
+      const emptyTaxonomies = taxonomiesList.filter(
+        (t) => t.contentTags.length === 0,
+      ).sort((a, b) => a.name.localeCompare(b.name));


The sorting is working, however when it appears on the page, the tag counts also include implicit tags, so it looks like it is not sorted. Do we want to sort only on applied tags or both applied and implicit?

Fixed here b48a33a

ChrisChV · 2024-04-11T15:57:04Z

@yusuf-musleh Thanks for the review!

When checking a tag in edit mode and adding it in the edit mode, if you click "Cancel" it gets cleared however the checkbox stays checked.

I fixed both cases here 1cb261e. Thanks for the suggestion

bradenmacdonald · 2024-04-11T16:18:46Z

@ChrisChV It seems like this PR is getting pretty big - is it worth splitting it up into separate PRs?

ChrisChV · 2024-04-11T18:10:20Z

It seems like this PR is getting pretty big - is it worth splitting it up into separate PRs?

@bradenmacdonald Yes, you're right. I have separated the functionalities made by @yusuf-musleh and I created #947

Also I separated this #949

The other functionalities are linked to the new states created for the edit and read mode. It will be very difficult to separate them

ChrisChV · 2024-04-12T20:15:48Z

Update: It is necessary to merge this at the same time of: openedx/edx-platform#34490 CC @yusuf-musleh @xitij2000

yusuf-musleh

👍 @ChrisChV Great work! And thanks for splitting up the PR and addressing the comments. I've gone through it again, and confirmed that its functioning as expected. I've tested the other PRs indivisually as well to confirm they're working as expected.

I tested this: Tested based on PR description.
I read through the code
I checked for accessibility issues
~~Includes documentation~~

xitij2000 · 2024-04-17T08:19:17Z

I've tried to review this, but was having trouble yesterday since the requirements were pointing to a deleted, and I my devstack broke as a results. I see that has now been fixed, so will try reviewing again.

xitij2000

I got to this pretty late so am leaving the feedback so far. Will continue the review tomorrow.

xitij2000 · 2024-04-17T10:20:02Z

src/content-tags-drawer/messages.js

+  tagsSaveToastTextTypeAddedAndRemoved: {
+    id: 'course-authoring.content-tags-drawer.toast.added-removed',
+    defaultMessage: '{tagsAdded} tags added. {tagsRemoved} tags removed.',
+    description: 'Text of toast after save when the user added or removed tags.',
+  },


I don't see the point of having this be a separate thing. There is already one for adding and one for removing. If the text was "x tags added and y tags removed" then it might make sense, but right now you can just display the other two texts.

Updated here e06f1b5

xitij2000 · 2024-04-17T13:49:45Z

src/content-tags-drawer/ContentTagsDrawerHelper.jsx

I'm wondering if this should be a context instead of a hook. If this state is being reused at multiple levels, then I think you should make this a context so that it doesn't need to be passed around.
Having it as a hook will still cause it to be rereun in each component, especially if they're on different levels in the same hierarchy.

If it should be a hook, perhaps it can be broken down into smaller hooks, or functions so it's easier to follow.

Sure. I used a context. This update is here: dfbc6eb

xitij2000 · 2024-04-17T14:00:24Z

src/content-tags-drawer/ContentTagsCollapsible.scss

@@ -5,6 +5,10 @@
  .collapsible-trigger {
    border: none !important;
  }
+
+  .tags-tree {
+    font-size: 17px;


I think it's generally a bad idea to use px sizes since they don't zoom well, perhaps we can use the closest css class or rem value?

Updated here: a185fe7

xitij2000 · 2024-04-17T14:09:09Z

src/content-tags-drawer/ContentTagsDrawer.scss

Some of the css here can be replaced with classes:

min-vh-100

box-shadow-up-2

Updated here: a185fe7

codecov · 2024-04-17T19:20:13Z

Codecov Report

Attention: Patch coverage is 96.22642% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 92.06%. Comparing base (be71668) to head (8110861).
Report is 1 commits behind head on master.

Files	Patch %	Lines
...ntent-tags-drawer/ContentTagsCollapsibleHelper.jsx	85.00%	6 Missing ⚠️
...rc/content-tags-drawer/ContentTagsDrawerHelper.jsx	98.07%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #939      +/-   ##
==========================================
+ Coverage   92.02%   92.06%   +0.04%     
==========================================
  Files         686      685       -1     
  Lines       11953    12055     +102     
  Branches     2600     2627      +27     
==========================================
+ Hits        11000    11099      +99     
- Misses        917      920       +3     
  Partials       36       36

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

ChrisChV · 2024-04-18T17:31:46Z

@xitij2000 It's ready for another review. I need help to fix the CI, I don't know what to do to fix that CodeCov error

bradenmacdonald · 2024-04-18T17:33:30Z

@ChrisChV For the codecov error, you just have to try again later, or get kshitij to push your commits to the main repo instead of the open-craft fork, and open a new PR. But probably just wait. The problem is a global rate limit for all "non-token" users of codecov on GitHub.

xitij2000

Tested again and it looks good to merge!

Ping me again once openedx/edx-platform#34490 is merged and I'll try to merge soon after.

ormsbee · 2024-04-25T17:57:03Z

@ChrisChV: Could you please squash your commits and add a more descriptive commit message?

It contains different changes to achieve the reading and editing mode of the drawer tag: * Manage tags drawer footer with buttons added. * Creation of ContentTagsDrawerContext. * Creation of global state and global removed state to allow edit mode. * Update API client to match with openedx-learning 0.9.1: Save tags of multiple taxonomies; to save all tags added/removed on edit mode * Extract TagsTree and use it on the Tags Drawer. * Update TagsTree to allow edit mode. * Add a Toast on Tags Drawer; show the toast afert save. * Scrolling + sticky footer on tags drawer

openedx-webhooks · 2024-04-25T19:29:16Z

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

ChrisChV requested a review from a team as a code owner April 5, 2024 22:44

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

ChrisChV marked this pull request as draft April 5, 2024 22:44

ChrisChV changed the title ~~Chris/fal 3645 refined tag drawer~~ [FC-0036] Refined tag drawer Apr 5, 2024

yusuf-musleh mentioned this pull request Apr 9, 2024

[FC-0036] feat: tags drawer style refinements open-craft/frontend-app-authoring#32

Merged

This was referenced Apr 10, 2024

Bump openedx-learning to support tagging with multiple taxonomies at once [FC-0036] openedx/edx-platform#34490

Merged

[FC-0036] feat: Update ObjectTagView openedx/openedx-learning#180

Merged

yusuf-musleh reviewed Apr 11, 2024

View reviewed changes

ChrisChV mentioned this pull request Apr 11, 2024

[FC-0036] Refined tag drawer UI style #947

Merged

ChrisChV force-pushed the chris/FAL-3645-refined-tag-drawer branch from b48a33a to 85fa126 Compare April 11, 2024 18:02

ChrisChV marked this pull request as ready for review April 12, 2024 20:10

yusuf-musleh approved these changes Apr 14, 2024

View reviewed changes

ChrisChV mentioned this pull request Apr 15, 2024

[Tagging] Refined Tag Drawer openedx/modular-learning#190

Closed

bradenmacdonald requested a review from xitij2000 April 15, 2024 20:38

xitij2000 reviewed Apr 17, 2024

View reviewed changes

ChrisChV mentioned this pull request Apr 17, 2024

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

Closed

bradenmacdonald mentioned this pull request Apr 18, 2024

[FC-0049] feat: Remove taxonomy export id prompt #955

Merged

ChrisChV force-pushed the chris/FAL-3645-refined-tag-drawer branch from 2e5c81f to cdaf7b5 Compare April 22, 2024 15:38

xitij2000 approved these changes Apr 23, 2024

View reviewed changes

ChrisChV force-pushed the chris/FAL-3645-refined-tag-drawer branch from b491d9b to 8110861 Compare April 25, 2024 19:12

ormsbee merged commit 9327948 into openedx:master Apr 25, 2024
6 checks passed

ChrisChV mentioned this pull request Apr 25, 2024

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

Merged

bradenmacdonald deleted the chris/FAL-3645-refined-tag-drawer branch April 29, 2024 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FC-0036] Refined tag drawer #939

[FC-0036] Refined tag drawer #939

ChrisChV commented Apr 5, 2024 •

edited

Loading

openedx-webhooks commented Apr 5, 2024 •

edited

Loading

yusuf-musleh left a comment

yusuf-musleh Apr 11, 2024

ChrisChV Apr 11, 2024

ChrisChV commented Apr 11, 2024 •

edited

Loading

bradenmacdonald commented Apr 11, 2024

ChrisChV commented Apr 11, 2024

ChrisChV commented Apr 12, 2024

yusuf-musleh left a comment

xitij2000 commented Apr 17, 2024

xitij2000 left a comment

xitij2000 Apr 17, 2024

ChrisChV Apr 17, 2024

xitij2000 Apr 17, 2024

xitij2000 Apr 17, 2024

ChrisChV Apr 17, 2024

xitij2000 Apr 17, 2024

ChrisChV Apr 17, 2024

xitij2000 Apr 17, 2024

ChrisChV Apr 17, 2024

codecov bot commented Apr 17, 2024 •

edited

Loading

ChrisChV commented Apr 18, 2024

bradenmacdonald commented Apr 18, 2024

xitij2000 left a comment

ormsbee commented Apr 25, 2024

openedx-webhooks commented Apr 25, 2024

[FC-0036] Refined tag drawer #939

[FC-0036] Refined tag drawer #939

Conversation

ChrisChV commented Apr 5, 2024 • edited Loading

Description

Supporting information

Testing instructions

openedx-webhooks commented Apr 5, 2024 • edited Loading

yusuf-musleh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisChV commented Apr 11, 2024 • edited Loading

bradenmacdonald commented Apr 11, 2024

ChrisChV commented Apr 11, 2024

ChrisChV commented Apr 12, 2024

yusuf-musleh left a comment

Choose a reason for hiding this comment

xitij2000 commented Apr 17, 2024

xitij2000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 17, 2024 • edited Loading

Codecov Report

ChrisChV commented Apr 18, 2024

bradenmacdonald commented Apr 18, 2024

xitij2000 left a comment

Choose a reason for hiding this comment

ormsbee commented Apr 25, 2024

openedx-webhooks commented Apr 25, 2024

ChrisChV commented Apr 5, 2024 •

edited

Loading

openedx-webhooks commented Apr 5, 2024 •

edited

Loading

ChrisChV commented Apr 11, 2024 •

edited

Loading

codecov bot commented Apr 17, 2024 •

edited

Loading