[FC-0036] Tags Sidebar #852

ChrisChV · 2024-02-22T20:41:24Z

Description

Adds a new tag sidebar. Important: This PR uses #832 as base, if you want to review the tag sidebar changes you can review this commits

Screenshots

Testing instructions

Review the commits added by this change: https://github.com/openedx/frontend-app-course-authoring/pull/852/files/09bf179e2828c261244cd86822e7f99426901c91..1d505dad132b3194ae3326e4b59e01b69672e531
Enable the new Unit page by adding a waffle flag contentstore.new_studio_mfe.use_new_unit_page in the CMS admin panel.
Make sure that the MFE setting ENABLE_UNIT_PAGE=true is enabled.
Go to the Course Unit page from the Course Outline page.
Verify the new tags sidebar.
Open the tags drawer and add some tags. Verify that the tags sidebar is updated in real time.
Verify the styles of the tags sidebar.
Open the tags drawer and delte all tags.
Verify the opacity of tags count when is zero.

Support information

Closes [Tagging] New Tag Sidebar Widget - Unit Page (in MFE) modular-learning#187
Internal ticket: FAL-3642

openedx-webhooks · 2024-02-22T20:41:30Z

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.

rpenido

Hi @ChrisChV!
Good work here!
I only added some missing type checks and nits.

Let me know what you think!

src/content-tags-drawer/tags-sidebar/TagsSidebarBody.jsx

src/content-tags-drawer/messages.js

rpenido · 2024-02-27T16:01:08Z

src/course-unit/CourseUnit.jsx

+                <Sidebar variant="publish" />
+                <Sidebar variant="tags" />
+                <Sidebar variant="location" />


I think these should be different components. As this is coming from upstream, what do you think about separating at least the TagsSideBar?

I also thought the same thing, but then I saw that they shared the same styles in different places, that's why I decided to integrate it CC @bradenmacdonald

That seems weird to me. Why not just make <Sidebar> into a wrapper component, so we can use

<Sidebar><PublishControls /></Sidebar> <Sidebar><TagControls /></Sidebar> <Sidebar><LocationInfo /></Sidebar>

or something like that? Of course you need to discuss with the other authors of the upstream PR. And maybe do that later as a follow-on. But that feels a lot cleaner to me.

rpenido · 2024-02-27T16:14:04Z

src/content-tags-drawer/ContentTagsDrawer.jsx

+  const params = useParams();
+  let contentId = id;
+
+  if (contentId === undefined) {
+    contentId = params.contentId;
+  }


I didn't like this, but I don't know a better way to do it and maintain compatibility with the iframe in the studio. Could you comment on why we use the component props and the query params?

I think this is fine for now, but I agree we should add a comment saying that once the legacy iframe is no longer used, we can remove useParams()

src/content-tags-drawer/tags-sidebar/TagsSidebarBody.jsx

src/content-tags-drawer/tags-sidebar/TagsSidebarHeader.jsx

src/content-tags-drawer/tags-sidebar/TagsTree.jsx

codecov · 2024-03-04T17:43:00Z

Codecov Report

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

Project coverage is 91.93%. Comparing base (6baec5b) to head (c9261e4).
Report is 3 commits behind head on master.

Files	Patch %	Lines
src/content-tags-drawer/data/apiHooks.jsx	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #852      +/-   ##
==========================================
+ Coverage   91.86%   91.93%   +0.06%     
==========================================
  Files         561      568       +7     
  Lines        9725     9844     +119     
  Branches     2079     2098      +19     
==========================================
+ Hits         8934     9050     +116     
- Misses        764      767       +3     
  Partials       27       27

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

xitij2000 · 2024-03-08T12:28:06Z

@ChrisChV Could you rebase this PR and fix the conflicts?

ChrisChV · 2024-03-11T20:39:43Z

@xitij2000 Done 👍 It's ready

xitij2000 · 2024-03-12T11:28:13Z

@ChrisChV Do I need a specific edx-platform PR for this?

ChrisChV · 2024-03-12T15:37:49Z

Do I need a specific edx-platform PR for this?

@xitij2000 sorry, if is about this issue, is fixed in openedx/edx-platform#34055. You need to pull your master branch

rpenido

@ChrisChV Nice work refactoring the components! LGTM 👍

I tested this in my local tutor dev following the PR instructions
I read through the code
I checked for accessibility issues
Includes documentation

src/content-tags-drawer/data/api.test.js

Co-authored-by: Rômulo Penido <[email protected]>

xitij2000

This is working well, however, I think the manage tags UI is a bit inconsistent.

As you can see in the screenshot below, the expand/contract icons are not well aligned, and neither are the tags.

If that is part of a future refinement task then do tell. Other than that things seem fine.

src/course-unit/sidebar/index.jsx

xitij2000 · 2024-03-12T09:19:57Z

src/content-tags-drawer/tags-sidebar-controls/TagsSidebarControls.scss

I think most if not all of the styling here can be applied via paragon / bootstrap classes.

Updated here 94d3f1f
I have left the styles that are applied to the internal components of the collapsible

ChrisChV · 2024-03-13T14:42:48Z

@xitij2000 It's ready for merge

arbrandes · 2024-03-13T15:02:07Z

src/content-tags-drawer/messages.js

+  },
+  tagsSidebarTitle: {
+    id: 'course-authoring.course-unit.sidebar.tags.title',
+    defaultMessage: 'Unit Tags',


It would be awesome if you could add some descriptions to these new messages. We'll be making them a requirement pretty soon. See openedx/frontend-build#517.

Done c9261e4

ChrisChV · 2024-03-14T15:06:22Z

If that is part of a future refinement task then do tell. Other than that things seem fine.

@xitij2000 Yes, all about the drawer is part of this task: openedx/modular-learning#190

openedx-webhooks · 2024-03-15T16:29:31Z

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

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

ChrisChV marked this pull request as draft February 22, 2024 20:45

ChrisChV marked this pull request as ready for review February 26, 2024 21:28

ChrisChV requested a review from a team as a code owner February 27, 2024 15:12

rpenido suggested changes Feb 27, 2024

View reviewed changes

ChrisChV mentioned this pull request Feb 28, 2024

[UU-58] Implement tagging & taxonomy feature in outline #855

Merged

ChrisChV force-pushed the chris/FAL-3642-tags-sidebar branch from 56aefd1 to 1d505da Compare March 4, 2024 17:33

ChrisChV mentioned this pull request Mar 5, 2024

feat: [FC-0044] Course unit page - Display sidebar component #832

Merged

ChrisChV added 12 commits March 11, 2024 11:24

refactor: Unit sidebar to create the TagsSidebar

88b5438

feat: Structure of TagsSidebar and TagsTree

6839859

feat: Adding styles to the TagsTree

4bed61a

feat: TagsSidebarHeader created

540c2f7

feat: Add count on TagsSidebarHeader

29ad22a

test: Tests for new components added

56af892

style: Update tags count with opacity when the count is zero

f808a84

fix: Warnings on console

139e978

refactor: Extract tag count component as generic

14fd7cd

chore: Fix rebase conflicts and issues

67cfe58

style: Fix nits

c83c2bc

style: Comments

7a70d81

ChrisChV force-pushed the chris/FAL-3642-tags-sidebar branch from 845e5ed to 7a70d81 Compare March 11, 2024 17:04

refactor: Transform Sidebar to a wrapper component

bc39fe9

fix: Issue with classnames

1f33ca4

rpenido approved these changes Mar 12, 2024

View reviewed changes

src/content-tags-drawer/data/api.test.js Outdated Show resolved Hide resolved

style: Typo on api.test.js

040e55b

Co-authored-by: Rômulo Penido <[email protected]>

xitij2000 approved these changes Mar 13, 2024

View reviewed changes

style: Update TagsSidebar styles

94d3f1f

ChrisChV requested a review from a team as a code owner March 13, 2024 14:37

chore: Fix merge conflicts

fa1c2d2

arbrandes reviewed Mar 13, 2024

View reviewed changes

feat: add description to comments

c9261e4

xitij2000 merged commit d57ecc6 into openedx:master Mar 15, 2024
6 checks passed

bradenmacdonald deleted the chris/FAL-3642-tags-sidebar branch March 18, 2024 01:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FC-0036] Tags Sidebar #852

[FC-0036] Tags Sidebar #852

ChrisChV commented Feb 22, 2024 •

edited

Loading

openedx-webhooks commented Feb 22, 2024 •

edited

Loading

rpenido left a comment

rpenido Feb 27, 2024

ChrisChV Mar 4, 2024

bradenmacdonald Mar 4, 2024

rpenido Feb 27, 2024

bradenmacdonald Mar 4, 2024

ChrisChV Mar 4, 2024

codecov bot commented Mar 4, 2024 •

edited

Loading

xitij2000 commented Mar 8, 2024

ChrisChV commented Mar 11, 2024

xitij2000 commented Mar 12, 2024

ChrisChV commented Mar 12, 2024

rpenido left a comment

xitij2000 left a comment

xitij2000 Mar 12, 2024

ChrisChV Mar 13, 2024

ChrisChV commented Mar 13, 2024

arbrandes Mar 13, 2024

ChrisChV Mar 14, 2024

ChrisChV commented Mar 14, 2024

openedx-webhooks commented Mar 15, 2024

[FC-0036] Tags Sidebar #852

[FC-0036] Tags Sidebar #852

Conversation

ChrisChV commented Feb 22, 2024 • edited Loading

Description

Screenshots

Testing instructions

Support information

openedx-webhooks commented Feb 22, 2024 • edited Loading

rpenido 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

codecov bot commented Mar 4, 2024 • edited Loading

Codecov Report

xitij2000 commented Mar 8, 2024

ChrisChV commented Mar 11, 2024

xitij2000 commented Mar 12, 2024

ChrisChV commented Mar 12, 2024

rpenido left a comment

Choose a reason for hiding this comment

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

ChrisChV commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisChV commented Mar 14, 2024

openedx-webhooks commented Mar 15, 2024

ChrisChV commented Feb 22, 2024 •

edited

Loading

openedx-webhooks commented Feb 22, 2024 •

edited

Loading

codecov bot commented Mar 4, 2024 •

edited

Loading