feat: View/Edit tags for Library Components

[yusuf-musleh]

Description

This PR adds the ability to view/edit tags for component blocks of V2 Libraries. This utilizes the already implemented content tags drawer in the course authoring MFE repo, rendering it inside an iframe.

If blocks have tags applied, the count is shown and can be clicked to open the manage tags drawer. Otherwise the manage tags option is available in the actions dropdown menu.

Supporting Information

Related Ticket:

• Resolves [Tagging] View/edit tags for a library component openedx/modular-learning#121
• Depends on feat: Add v2 lib components content tags support openedx/frontend-app-authoring#771
• Depends on feat: Add tags count for v2 library components openedx/edx-platform#33979

Testing Instructions

1. Checkout feat: Add v2 lib components content tags support openedx/frontend-app-authoring#771 and run it locally
2. Checkout feat: Add tags count for v2 library components openedx/edx-platform#33979 and run it locally
3. Run this PR locally
4. Make sure you have sample taxonomy data locally, check out https://github.com/open-craft/taxonomy-sample-data/ if needed
5. Navigate to http://localhost:3001/ and create a new V2 Library if it doesnt already exist
6. Navigate to the newly created Library and add a few blocks/components to it
7. Click on the 3-dotted more actions button in one of the blocks/components, and select Mange tags, the side drawer should open and load taxonomy tags to choose from
8. Try adding/removing tags then close the block
9. Refresh the page, you should notice a button with currently applied tags count on it, clicking on it should also open up the side drawer. Note: The count currently only shows applied tags count, not including the implicit tags. The implicit tags count implementation in the latest openedx-learning version that has not been released yet.

---

Private-ref: FAL-3599

[openedx-webhooks]

Thanks for the pull request, @yusuf-musleh! 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]

Codecov Report

Attention: 22 lines in your changes are missing coverage. Please review.

Comparison is base (f565521) 52.41% compared to head (a33e6ef) 53.14%.

Files	Patch %	Lines
...oring/author-library/BlockPreviewContainerBase.jsx	71.79%	11 Missing ⚠️
...-authoring/author-library/LibraryAuthoringPage.jsx	84.61%	4 Missing ⚠️
...rary-authoring/author-library/BlockPreviewBase.jsx	86.36%	3 Missing ⚠️
...ary-authoring/author-library/ContentTagsDrawer.jsx	92.30%	2 Missing ⚠️
.../author-library/LibraryAuthoringPageHeaderBase.jsx	87.50%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #400      +/-   ##
==========================================
+ Coverage   52.41%   53.14%   +0.72%     
==========================================
  Files          75       80       +5     
  Lines        1986     2021      +35     
  Branches      359      368       +9     
==========================================
+ Hits         1041     1074      +33     
- Misses        912      914       +2     
  Partials       33       33              

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

[rpenido]

Hi @yusuf-musleh !

I think we can simplify a bit this code rendering the ContentTagsDrawer conditionaly instead of hiding it using styles.

I made some tests in my stack and it worked. Let me know if you see any issues with this approach!

[rpenido]

With position fixed, the drawer worked fine without this.

Suggested change

	body.drawer-open {
	overflow: hidden;
	}

[yusuf-musleh]

This one is needed otherwise the user will be able to scroll down in the darkened part of the page.

[rpenido]

Understood! Would you mind putting a comment in the CSS mentioning it? 😃

[yusuf-musleh]

Done, updated the commit

[yusuf-musleh]

@rpenido Thanks for the review! Your suggestions are great and much cleaner. I applied all of them (e0db5b7) except 1, left a comment about it above.

I also added auto focus to the drawer when it opens so that it is directly accessible through the keyboard when it is opened, it works fine when I open it through the tags count button, however when I open it through the dropdown action menu, the focus still remains on the main page for some reason. Any ideas on how to approach this?

  const autoFocusDrawer = (drawer) => {
    // Set focus to drawer when its opened
    document.activeElement.blur();
    if (drawer) {
      drawer.focus();
    }
  };

  // TODO: The use of an iframe in the implementation will likely change
  const renderIFrame = () => (
    <iframe
      ref={autoFocusDrawer}
      title="manage-tags-drawer"
      className="w-100 h-100 border-0"
      src={`${getConfig().COURSE_AUTHORING_MFE_BASE_URL}/tagging/components/widget/${openContentTagsDrawer}`}
    />
  );

[rpenido]

@rpenido Thanks for the review! Your suggestions are great and much cleaner. I applied all of them (e0db5b7) except 1, left a comment about it above.

I also added auto focus to the drawer when it opens so that it is directly accessible through the keyboard when it is opened, it works fine when I open it through the tags count button, however when I open it through the dropdown action menu, the focus still remains on the main page for some reason. Any ideas on how to approach this?

The following code works for the button and the menu. useRef is imported from React.

  const iFrameRef = useRef();

  useEffect(() => {
    if (openContentTagsDrawer && iFrameRef.current) {
      iFrameRef.current.focus();
    }
  }, [openContentTagsDrawer]);

  // TODO: The use of an iframe in the implementation will likely change
  const renderIFrame = () => (
    <iframe
      ref={iFrameRef}
      title="manage-tags-drawer"
      className="w-100 h-100 border-0"
      src={`${getConfig().COURSE_AUTHORING_MFE_BASE_URL}/tagging/components/widget/${openContentTagsDrawer}`}
    />
  );

However, this does not prevent the user from focusing (via TAB) items behind the overlay. Maybe the ModalLayer from paragon (https://paragon-openedx.netlify.app/components/modal/modal-layer/) would make it easier, handling the autofocus, scroll of the background, overlay, etc.. because it is already implemented.

  return (
    <ModalLayer isOpen={!!openContentTagsDrawer}>
      <div role="dialog" id="manage-tags-drawer" className="drawer">
        { renderIFrame() }
      </div>
    </ModalLayer>
  );

If you think this is getting bigger, maybe we should drop it and make this refinement later.

[yusuf-musleh]

The following code works for the button and the menu. useRef is imported from React.

@rpenido Your suggested worked really well, thanks! It correctly set the focus to the drawer when it opens from either the tags count button or the menu button, I've updated the code to include your suggestion. I think for now this should be fine, if we need to make adjustments to it, we can do in a follow up PR.

[yusuf-musleh]

@bradenmacdonald This is ready for CC review, not sure who we need to ping to do that?

[bradenmacdonald]

Hmm, according to the chart, OpenCraft doesn't have any core contributors for this repo. And since we're planning to consolidate this into course-authoring, I don't think it's worth onboarding anyone either.

@arbrandes or @kdmccormick do either of you have availability to do a Core Contributor review + merge of this PR for the tagging project?

[rpenido]

Hi @yusuf-musleh!
Could we change this var to COURSE_AUTHORING_MICROFRONTEND_URL?

The tutor-mfe plugin already define it by default:
overhangio/tutor-mfe#182

[yusuf-musleh]

@rpenido Sure thing, updated here: 83f12d8

[ali-hugo]

@yusuf-musleh Please move "Manage Tags" above "Delete" in the overflow menu - just to keep it in line with tagging Units.

[yusuf-musleh]

Please move "Manage Tags" above "Delete" in the overflow menu - just to keep it in line with tagging Units.

@ali-hugo Sure thing, will update it shortly.

[yusuf-musleh]

@ali-hugo Updated the order of the actions in the dropdown menu.

[itsjeyd]

Hey @arbrandes and @kdmccormick, a friendly nudge about @bradenmacdonald's comment above:

... do either of you have availability to do a Core Contributor review + merge of this PR for the tagging project?

[navinkarkera]

@yusuf-musleh Nice work! Left a small question.

Not related to this PR but the taxonomy sample data generate script fails due to removal of tag_content_object in this commit, had to checkout the previous commit and run the sample generation script.

• I tested this: (in devstack locally)
• I read through the code
• I checked for accessibility issues
• Includes documentation

[navinkarkera]

@yusuf-musleh I forgot to include this before in my comment, but can we split src/library-authoring/author-library/LibraryAuthoringPage.jsx file? It is too big right now, we can do it in the next pull request if it is too much work.

[yusuf-musleh]

@navinkarkera Good point, I've split the LibraryAuthoringPage.jsx to multiple components. All tests are passing, however the CLA check is still waiting, is this a new check that was added recently? I don't remember this was here in my previous commits to this PR.

cc @kdmccormick @bradenmacdonald

[kdmccormick]

@yusuf-musleh the CLA check is not new, but assuming that you are an OpenCraft employee, it should pass no problem. If it doesn't turn green within a few hours, ping me again and I'll look into why it isn't reporting.

[kdmccormick]

Nevermind, looks like it just passed.

[yusuf-musleh]

@kdmccormick Oh interesting, probably just took longer this time, thanks for the insight!

[navinkarkera]

@yusuf-musleh Thanks for splitting it. Please rebase.

[yusuf-musleh]

@navinkarkera Rebased and good to go, thanks again for the review!

[openedx-webhooks]

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