-
Notifications
You must be signed in to change notification settings - Fork 81
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
[UU-58] Implement tagging & taxonomy feature in outline #855
[UU-58] Implement tagging & taxonomy feature in outline #855
Conversation
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:
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. |
@navinkarkera Is this also some of the work that you were planning on doing for the outline workstream? |
7c3e176
to
16a62df
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #855 +/- ##
==========================================
+ Coverage 90.91% 90.97% +0.05%
==========================================
Files 533 535 +2
Lines 9256 9294 +38
Branches 1946 1958 +12
==========================================
+ Hits 8415 8455 +40
+ Misses 809 807 -2
Partials 32 32 ☔ View full report in Codecov by Sentry. |
@KristinAoki Yes. @ChrisChV has offered to help with it. |
src/generic/tag-count/index.jsx
Outdated
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 have created this component as generic and with extra functionalities (e.g. changing opacity), because it will be used in other interfaces. E.g #852
4d2c9a2
to
1912890
Compare
{onClickManageTags && ( | ||
<Dropdown.Item | ||
data-testid={`${namePrefix}-card-header__menu-manage-tags-button`} | ||
onClick={onClickManageTags} | ||
> | ||
{intl.formatMessage(messages.menuManageTags)} | ||
</Dropdown.Item> | ||
)} |
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.
Do we need the dropdown item and the tag count to both have onClick
attributes? Should the tag count be read only?
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.
Yes, the tag count will be read only in other pages. E.g #852
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.
Ok, so you will update the tag count in the header to be read only and the dropdown item will have to onclick attribute.
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.
In the course outline it is necessary that the Manage tags
menu (dropdown item) and the tag count component can open the Manage tags drawer, for that reason they both have the onclick attribute (Ref)
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.
@ChrisChV Nice work! 👍
- I tested this: (add/delete tags via new menu option)
- I read through the code
- I checked for accessibility issues
-
Includes documentation
@@ -127,6 +130,7 @@ const CardHeader = ({ | |||
{(isVertical || isSequential) && ( | |||
<CardStatus status={status} showDiscussionsEnabledBadge={showDiscussionsEnabledBadge} /> | |||
)} | |||
{ tagsCount !== undefined && tagsCount !== 0 && <TagCount count={tagsCount} onClick={onClickManageTags} /> } |
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.
{ tagsCount !== undefined && tagsCount !== 0 && <TagCount count={tagsCount} onClick={onClickManageTags} /> } | |
{ tagsCount > 0 && <TagCount count={tagsCount} onClick={onClickManageTags} /> } | |
I've created issue TNL-11474 in the private 2U Jira. |
@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. |
Description
Adds the
Manage tags
menu item andTagCount
button to Units.Screenshots
Supporting information
Testing instructions
contentstore.new_studio_mfe.use_new_course_outline_page
waffle flagManage tags
. Verify that it opens the tag drawer.