[FC-0036] feat: Add ContentTagsDrawer widget - #654
Conversation
openedx-webhooks
added
the
open-source-contribution
|
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:
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
changed the title
|
Hi @yusuf-musleh! Would you be able to provide detail / additional context for this contribution? You can put this information at the top of the PR. Thanks! |
mphilbrick211
added
the
waiting on author
11610c4 to
4a6b8de
Compare
yusuf-musleh
changed the title
yusuf-musleh
changed the title
|
@mphilbrick211 Sure thing! I've updated the PR with some information for context. I will add the full PR description once its closer to review, as it is still a work in progress. |
mphilbrick211
added
needs test run
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #654 +/- ##
==========================================
+ Coverage 88.46% 88.61% +0.15%
==========================================
Files 424 434 +10
Lines 6587 6766 +179
Branches 1419 1451 +32
==========================================
+ Hits 5827 5996 +169
- Misses 735 745 +10
Partials 25 25 ☔ View full report in Codecov by Sentry. |
|
@yusuf-musleh looks like there are some coverage issues, can you have a look? |
e0d
removed
the
needs test run
|
@mphilbrick211 This is actually part of FC-0036. Do you know why there's no "FC" label for PRs in this repo? |
I'm not sure. I don't have the ability to add the labels across repos, so I'll need to check on that internally. |
| }; | ||
|
|
||
| const useTaxonomyListData = () => { | ||
| const taxonomyListData = useTaxonomyListDataResponse(org); |
There was a problem hiding this comment.
I don't think the "taxonomy list" is actually necessary for the read-only view, though it's fine that you've implemented it because we'll need it in the editable version.
For the read-only version, the "get object tags" API returns the name (Taxonomy name) for each tag, which should be enough to display the view. (For the read-only version, we don't need to display taxonomies that don't yet have any tags.) And for displaying the ancestor tags, I'm working on an update to the API to make that possible, so it can be out of scope for now.
Sorry I didn't catch this sooner when we were discussing pagination of the API, as I realize it increased the scope of this ticket for you.
There was a problem hiding this comment.
In fact, I think we don't need to wait for the taxonomy list to load before displaying anything here. We could just call "get object tags" and immediately render whatever tags+taxonomies it returns, and at the same time (or later) we can call the "list taxonomies" and then for any taxonomies which don't yet have tags, we could append them to the list. No need to change this now though.
There was a problem hiding this comment.
Got it, thanks for the pointers, I'll keep this in mind when implementing the editing editable version.
d2db1ad to
8d19c00
Compare
yusuf-musleh
changed the title
| ariaLabel="taxonomy tags selection" | ||
| className="taxonomy-tags-selectable-box-set" | ||
| > | ||
| {isTaxonomyTagsLoaded && taxonomyTagsData && taxonomyTagsData.results.map((taxonomyTag) => ( |
There was a problem hiding this comment.
We can change this in the next PR (editing), but IMHO the list of available tags should not be loaded unless the user clicks on the "Add Tags" button. Because more often than not, they'll just be viewing the tags, not editing them, so we don't need to load the list.
I would recommend moving the loading logic (taxonomyTagsData) into <TaxonomyTagsDropDownSelector> and only triggering it if the user has click on "Add Tags" and wants to see the list of selected/additional tags. Again, you can do that in a future PR, so maybe just add a comment about this in the code for now.
There was a problem hiding this comment.
Thought I replied to this, my bad! Yes, agreed, I've added a comment and will address it in the editing PR.
| let level = -1; // Starting from -1 since we increment at beginning of loop | ||
| return unitTag.lineage.map((lineageTag) => { | ||
| level += 1; | ||
| return (<TagBubble key={`tag-${lineageTag}`} value={lineageTag} implicit={lineageTag !== unitTag.value} level={level} />); |
There was a problem hiding this comment.
This is not rendering multiple tags properly. (BTW we should change the lightcast skills taxonomy to allow_multiple=True and make it apply to all orgs in the sample data.) I suggest that you make this component so that it accepts the existing object tags data for this single taxonomy as a prop, and it also has an "additional selected tags" as internal state (which will get implemented by the "Add Tags" button), then you create a function to combine these together into a tree:
// appliedObjectTags is a prop, which has the data from the content_object_tags REST API, but only for this current taxonomy.
const [changedTags, setChangedTags] = React.useState(...);
const tagsTree = React.useMemo(() => {
// compute the tree from appliedObjectTags, using the lineage
// add in any newly added tags (that haven't yet been saved to the server) - can come in next PR
// remove any deleted tags - can come in next PR
// delete any "orphaned" parents with no children - can come in next PR
return tagsTree;
}, [appliedObjectTags, changedTags]);
// render the view using tagsTreeHow this looks now with multiple tags:
How it should look:
There was a problem hiding this comment.
Oh thanks for catching this! It makes sense to create a separate component to handle this logic.
(BTW we should change the lightcast skills taxonomy to allow_multiple=True and make it apply to all orgs in the sample data.)
Yup, I agree, I'll update the sample data script and create a PR once I've addressed all the issues.
There was a problem hiding this comment.
@bradenmacdonald I've implemented properly rendering of multiple tags, and managed to fit it in as part of this PR f8cbc91 . Your suggestions worked great, and sets up up code for the the editing PR that follows.
|
@bradenmacdonald Thanks for the awesome review! I've addressed most of the issues. There are 2 things remaining that I didn't quite complete today, the rendering of multiple tags properly and the renaming. I agree it makes sense to rename this component. I also think it's worth renaming the actual folder/files and some variable names to be more generic rather than specific to "units" and dropping the "taxonomy" wording. I think doing it now before it gets merged would save us some headache down the road. |
|
Thanks @yusuf-musleh. I think this will be good to go once you have those done! Though I'm not the final reviewer here. |
|
@yusuf-musleh BTW I really want to get this merged ASAP, so I agree that renaming the components should happen before we merge, but if the "rendering multiple tags properly" is going to take a while, let's just address that in the next PR (the editing PR or even just a simple PR focused on that one fix). |
yusuf-musleh
changed the title
ad3d3cb to
811c874
Compare
xitij2000
left a comment
xitij2000
left a comment
There was a problem hiding this comment.
I think there is more use of custom SCSS than is needed here. There are also a couple of other smaller issues I've found.
| .taxonomy-tags-collapsible-div { | ||
| display: flex; | ||
| } | ||
|
|
||
| .taxonomy-tags-collapsible { | ||
| flex: 1; | ||
| border: none !important; | ||
|
|
||
| .collapsible-trigger { | ||
| border: none !important; | ||
| } | ||
| } | ||
|
|
||
| .tags-count-badge-div { | ||
| display: flex; | ||
| } | ||
|
|
||
| .tags-count-badge { | ||
| align-self: start; | ||
| margin-top: 1rem; | ||
| } | ||
|
|
||
| .tags-count-badge-empty { | ||
| visibility: hidden; | ||
| } | ||
|
|
||
| .taxonomy-tags-selector-menu { | ||
| display: flex; | ||
|
|
||
| button { | ||
| flex: 1; | ||
| } | ||
| } | ||
|
|
||
| .taxonomy-tags-selector-menu + div { | ||
| width: 100%; | ||
| } | ||
|
|
||
| .taxonomy-tags-selectable-box-set { | ||
| grid-auto-rows: unset !important; | ||
| overflow-y: scroll; | ||
| max-height: 20rem; | ||
| } |
There was a problem hiding this comment.
A lot of the custom css here can be replaced by paragon/bootstrap classes:
display: flex-> d-flexalign-self: start-> align-self-startwidth: 100%-> w-100
There was a problem hiding this comment.
Got it, I've updated most of them to use the paragon/bootstrap classes. 12df985 The width: 100% in this case set the style for an element that is added on render from the paragon component so I could not add to the code.
| // Each index contains whether the dropdown is open/closed | ||
| // O === Closed | ||
| // 1 === Open | ||
| const [dropdownStates, setDropdownStates] = useState([]); |
There was a problem hiding this comment.
I think some of this code could be simplified if this an object mapping the index to true/false/undefined.
isOpen can stay the same.
Updating an index can be setDropDownStates({...dropdownState, i:!dropdownState[i]})
You don't need to initialize since initial keys are undefined which are falsy.
There was a problem hiding this comment.
Thanks for the great suggestion! Indeed, it simplified the code quite a bit: d17501e
| return ( | ||
| isTaxonomyTagsLoaded && taxonomyTagsData | ||
| ? taxonomyTagsData.results.map((taxonomyTag, i) => ( | ||
| <div style={{ display: 'flex', flexDirection: 'column', paddingLeft: `${level * 1}rem` }}> |
There was a problem hiding this comment.
You should use classes for some of this.
d-flex flex-column
As for the padding, if it is nested and each keeps a margin you won't need to increase padding at each level?
There was a problem hiding this comment.
I've replaced them with classes as you suggested. However I kept the paddingLeft since it doesn't add any margin/padding (even though it is nested) so they all appear on the same level without it.
| <SelectableBox | ||
| inputHidden={false} | ||
| type="checkbox" | ||
| className="pgn__selectable_box taxonomy-tags-selectable-box" |
There was a problem hiding this comment.
Why is pgn__selectable_box here, shouldn't it be applied automatically? Is there a bug?
There was a problem hiding this comment.
You're right! I removed it. I think I initially added it there to make it more explicit since I override some of it's styles.
| @@ -0,0 +1,9 @@ | |||
| .tag-bubble.btn-outline-dark:hover { | |||
| color: white; | |||
| background-color: #273F2F; | |||
There was a problem hiding this comment.
This corresponds with bg-dark, or the color $dark.
| } | ||
|
|
||
| .implicit > .btn-icon-before svg { | ||
| color: #E9E6E4; // $light-400; |
There was a problem hiding this comment.
Why not just use $light-400?
There was a problem hiding this comment.
I tried using the variables like $light-400 but it would not work. I kept getting errors in the build that it was not defined. I'm not sure what I am doing differently compared to the other existing scss files that utilized variables?
There was a problem hiding this comment.
You are importing TagBubble.scss into the TagBubble component here, which is fine, except that your .scss file doesn't import the SCSS variables nor Paragon base classes etc. And in fact you don't want to, because that will result in a double build and duplicated Paragon classes in the final CSS.
What you need to do instead is remove the reference to TagBubble.scss from your .jsx file, and instead include the custom styles via the master SCSS file, https://github.com/openedx/frontend-app-course-authoring/blob/master/src/index.scss . For simplicity, usually that file includes something from the top-level folder which then in turn includes the smaller .scss files.
This is an "old-fashioned" system btw. The modern approach is to skip SCSS altogether and use CSS variables as well as relying much more heavily on Utility classes. I'm a big Tailwind fan and with that you have almost no styles at all in your global CSS and you have few or no separate CSS files. Generally you can achieve everything with utility classes like d-flex align-self-start etc. I don't think Paragon/boostrap's utility classes are as powerful, but the same principle applies - try to use utility classes as much as possible and create custom classes in the .css file only for very custom/unusual stuff where needed, like the Tag elements.
There was a problem hiding this comment.
oh I see - I followed your suggestions and it worked! I was getting confused as to why it wasn't working. Thanks for clearing it up for me.
There was a problem hiding this comment.
For what it's worth, once the design tokens bits land in paragon, the $light-400 will be replaced by a CSS variable anyway. There are automated tools to do that replacement in paragon alpha.
| // @ts-check | ||
| import { useTaxonomyTagsData, useContentTaxonomyTagsData, useContentData } from './api'; | ||
|
|
||
| /** | ||
| * @returns {import("../types.mjs").TaxonomyTagsData | undefined} | ||
| */ | ||
| export const useTaxonomyTagsDataResponse = (taxonomyId, fullPathProvided) => { | ||
| const response = useTaxonomyTagsData(taxonomyId, fullPathProvided); | ||
| if (response.status === 'success') { | ||
| return response.data.data; | ||
| } | ||
| return undefined; | ||
| }; | ||
|
|
||
| /** | ||
| * @returns {boolean} | ||
| */ | ||
| export const useIsTaxonomyTagsDataLoaded = (taxonomyId, fullPathProvided) => ( | ||
| useTaxonomyTagsData(taxonomyId, fullPathProvided).status === 'success' | ||
| ); | ||
|
|
||
| /** | ||
| * @returns {import("../types.mjs").ContentTaxonomyTagsData | undefined} | ||
| */ | ||
| export const useContentTaxonomyTagsDataResponse = (contentId) => { | ||
| const response = useContentTaxonomyTagsData(contentId); | ||
| if (response.status === 'success') { | ||
| return response.data; | ||
| } | ||
| return undefined; | ||
| }; | ||
|
|
||
| /** | ||
| * @returns {boolean} | ||
| */ | ||
| export const useIsContentTaxonomyTagsDataLoaded = (contentId) => ( | ||
| useContentTaxonomyTagsData(contentId).status === 'success' | ||
| ); | ||
|
|
||
| /** | ||
| * @returns {import("../types.mjs").ContentData | undefined} | ||
| */ | ||
| export const useContentDataResponse = (contentId) => { | ||
| const response = useContentData(contentId); | ||
| if (response.status === 'success') { | ||
| return response.data.data; | ||
| } | ||
| return undefined; | ||
| }; | ||
|
|
||
| /** | ||
| * @returns {boolean} | ||
| */ | ||
| export const useIsContentDataLoaded = (contentId) => ( | ||
| useContentData(contentId).status === 'success' | ||
| ); |
There was a problem hiding this comment.
I'll echo the same concerns I showed here.
If a function starts with use is it treated as a react hook by the linters. None of these need to be react hooks, so the "use" at the beginning can be dropped.
There was a problem hiding this comment.
I could be mistaken, but since the functions in /api/hooks/api.js return useQuery which is a hook, they also become hooks. If we drop the "use" in the functions in the /api/hooks/selectors.js that call them, I get linter errors saying that the hooks cannot be called inside normal functions and suggests to either convert them to hooks (the current case with "use") or convert them to React component functions which doesn't seem right? Maybe I am misunderstanding how to go about addressing this.
There was a problem hiding this comment.
My understanding was the same as @yusuf-musleh's - any function which uses a hook is itself a hook (or a component) so should be named use....
There was a problem hiding this comment.
@xitij2000 @bradenmacdonald I changed the structure of these hooks/apis to follow #645 that is using useQuery after pulling it from master. 49cca87
There was a problem hiding this comment.
My bad, I think I got confused switching between PRs. Sorry about this, I think this looks good.
|
@xitij2000 Thanks for the review! I addressed some of the issues, and had a few questions about some of the points. |
d1d1ee0 to
49cca87
Compare
|
Hi @yusuf-musleh, Apologies for the late review but I only came across this PR today. I have some feedback about the styling of the tags (these changes aren't at all urgent, but it would be good if they could be implemented sometime). Please see the tables below comparing the current styling (on the left) and the correct styling (on the right): IMPLICIT TAGS:
EXPLICIT TAGS:
Please let me know if you have any questions. |
e2dcf03 to
78d6cc9
Compare
|
@ali-hugo Thanks for the review and feedback. I made the changes regarding the Tag outline color. However for the implicit icon, the outline tag icon is not available in Paragon or in the Material Symbols and Icons from Google Fonts (that is used under the hood in Paragon). That's why i went with the different color that didn't match the design. However I did find it as part of a newer version of font-awesome (6.4.2) but it is considered a "Pro" icon. I'm not sure what is the general process is in this situation when the icon is not available? Have you faced this before @bradenmacdonald ? |
|
@yusuf-musleh Thanks for updating the outline color 🙂 . The outline tag icon is actually available in the Google Fonts icons - it's just a bit tricky to find. You have to ensure that the "fill" toggle in the filters drawer is off. You should be able to access the outline version of the icon at this link. I have also included the svg below:
Let me know if you run into any trouble. |
|
@ali-hugo ohh! I completely missed that! Thanks for pointing it out. I've added it as a custom tag and updated the style, here's how it looks like now (the filled in one is hovered on):
Ideally we'd create a PR and add the icon to Paragon, but we can leave that for a later task/ticket. |
|
@yusuf-musleh Yay, that looks great! Thank you 😄
Perhaps we should wait until we've done some user testing before we add the icon to Paragon. That way we can make sure that the chosen icon makes sense to users before we make it a permanent feature. Users never fail to surprise me with their feedback! |
xitij2000
left a comment
xitij2000
left a comment
There was a problem hiding this comment.
I have mostly just minor nits, other than that it looks good to go.
|
|
||
| const closeContentTagsDrawer = () => { | ||
| // "*" allows communication with any origin | ||
| window.parent.postMessage('closeManageTagsDrawer', '*'); |
There was a problem hiding this comment.
I don't there is any set standard for this, but a few other places this is used it's a string like: 'lmshtmlfragment.resize' and 'tools.toggleNotes'.
There was a problem hiding this comment.
Making this change would require also making changes to the edx-platform. I think so it doesn't get blocked by another review cycle there, we can address it in a later PR.
| const handleEsc = (event) => { | ||
| /* Close drawer when ESC-key is pressed and selectable dropdown box not open */ | ||
| const targetElement = event.target; | ||
| const selectableBox = targetElement.classList.contains('pgn__selectable_box'); |
There was a problem hiding this comment.
Could there be a better way of doing this? For instance perhaps adding a data attribute and matching that?
There was a problem hiding this comment.
While addressing this, I found a bug with closing the drawer when the dropdown is open. I updated it with a data-attr and fixed the issue. cda3a91
| const TagOutlineIcon = (props) => ( | ||
| <svg | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| height="24px" | ||
| viewBox="0 0 24 24" | ||
| width="24px" | ||
| fill="currentColor" | ||
| role="img" | ||
| focusable="false" | ||
| aria-hidden="true" | ||
| {...props} | ||
| > | ||
| <path d="M0 0h24v24H0V0z" fill="none" /> | ||
| <path | ||
| d="m21.41 11.58-9-9C12.05 2.22 11.55 2 11 2H4c-1.1 0-2 .9-2 2v7c0 .55.22 1.05.59 1.42l9 9c.36.36.86.58 1.41.58s1.05-.22 1.41-.59l7-7c.37-.36.59-.86.59-1.41s-.23-1.06-.59-1.42zM13 20.01 4 11V4h7v-.01l9 9-7 7.02z" | ||
| /> | ||
| <circle cx="6.5" cy="6.5" r="1.5" /> | ||
| </svg> | ||
| ); | ||
|
|
||
| export default TagOutlineIcon; |
There was a problem hiding this comment.
This should be moved to paragon right?
There was a problem hiding this comment.
Ideally yes, since this icon is not already in Paragon. But we've agreed to create a PR in paragon to add it once we've confirmed with the user that they liked it. #654 (comment)
954946e to
dd9ef5a
Compare
|
@xitij2000 Thanks for your review and great recommendations! I've addressed them and updated the PR. @bradenmacdonald This should be good go! |
|
@xitij2000 Thanks for the review! @yusuf-musleh I can't merge this; @xitij2000 could you please merge? |
|
@yusuf-musleh Could you squash this down to one or a few commits with a descriptive commit message? I will merge ASAP after that. |
dd9ef5a to
5af7513
Compare
|
@xitij2000 I squashed the commits and went through it another time locally, should be good to go to merge. |
Could you add a description of the commit that describes what it's adding? |
5af7513 to
c1100d2
Compare
Sure thing, updated. |
This implements a side drawer widget for content taxonomy tags. It includes displaying the object's tags, along with their lineage (ancestor tags) data. It also implements the listing the available taxonomy tags (including nesting ones) to select from to apply to this unit. Note: The editing of tags (adding/removing) will be added in a future PR. * feat: Add initial UnitTaxonomyTagsDrawer widget * feat: Add fetching unit taxonomy tags from backend * feat: Add fetching/group tags with taxonomies * feat: Add fetch Unit data and display name * feat: Add Taxonomy Tags dropdown selector * feat: Add TagBubble for tag styling * chore: Add distinct keys to elements + remove logs * feat: Add close drawer with ESC- keypress * feat: Make dropdown selectors keyboard accessible * chore: Fix issues causing validation to fail * test: Add coverage tests for UnitTaxonomyDrawer * feat: Incorporate tags lineage data from API * refactor: Remove/replace deprecated injectIntl * test: Remove redux store related code + fix warnings * feat: Use <Loading /> instead of loading string * docs: Add docs string to TaxonomyTagsCollapsible * feat: Use <Spinner/> to allow mutiple loading to show * feat: Rename UnitTaxonomyTagDrawer -> ContentTagsDrawer * feat: Add ContentTagsTree component to render Tags * feat: Only fetch tags when dropdowns are opened * refactor: Simply dropdown close/open states * feat: Use built in class styles instead of custom * feat: Replace hardcoded values with scss variables * refactor: follow existing structure for reactQuery/APIs * feat: Change tag bubble outline color * feat: Add TagOutlineIcon for implicit tags * feat: Make aria label internationalized * feat: Replace custom styles with builtin classes * fix: Fix bug with closing drawer * refactor: Simplify content tags fetching code * refactor: Simplify getTaxonomyListApiUrl
c1100d2 to
92934b6
Compare
|
@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. |
8 participants
Description
This PR implements a side drawer widget for units' taxonomy tags. It includes displaying the Unit's tags, along with their lineage (ancestor tags) data. It also implements the listing the available taxonomy tags (including nesting ones) to select from to apply to this unit.
Things to Note:
Screenshots
Supporting information
Related Tickets:
Related PRs:
Testing instructions
Private-ref: FAL-3523