feat: Tag List on Unit page [FC-0036] #33645
Conversation
openedx-webhooks
added
the
open-source-contribution
|
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. |
|
Sandbox deployment started. |
ChrisChV
changed the title
|
Sandbox update request received. Deployment will start soon. |
|
Sandbox deployment successful. Sandbox LMS is available at pr-33645-139931.staging.do.opencraft.hosting |
|
Sandbox deployment started. |
|
Sandbox deployment successful. Sandbox LMS is available at pr-33645-139931.staging.do.opencraft.hosting |
There was a problem hiding this comment.
@ChrisChV Overall I think this PR looks good, very clean code! I just had main question regarding the object tags grouping/lineage implementation, would love to hear your thoughts.
Also, I got confused initially because the side section UI changes were not showing. Spent some time figuring out why, it turns out I forgot I needed to run the paver watch_assets and paver compile_sass commands to see the reflected changed.
I think it would be helpful to include those as part of the testing instructions as well, especially if the reviewer hasn't worked major frontend work in this repo for some time.
| Get the tags of a Unit and build a json to be read by the UI | ||
| """ | ||
| # Get content tags from content tagging API | ||
| content_tags = get_content_tags(usage_key) |
There was a problem hiding this comment.
@ChrisChV I'm wondering if there is a way to make use of @bradenmacdonald 's updated implementation in the openedx-learning repo that added the lineage information to the object tags and takes care of the grouping under taxonomies as well, rather than re-implementing it here as well we could potentially utilize that as a single source of truth.
Here is a sample of how the data looks like:
{
"block-v1:SampleTaxonomyOrg1+STC1+2023_1+type@vertical+block@aaf8b8eb86b54281aeeab12499d2cb0b": {
"taxonomies": [
{
"name": "LightCastSkillsTaxonomy",
"taxonomy_id": 15,
"editable": true,
"tags": [
{
"value": "Administrative Functions",
"lineage": [
"Administration",
"Administrative Support",
"Administrative Functions"
]
}
]
},
{
"name": "OpenCanadaTaxonomy",
"taxonomy_id": 14,
"editable": true,
"tags": [
{
"value": "Verbal Ability",
"lineage": [
"Abilities",
"Cognitive Abilities",
"Communication Abilities",
"Verbal Ability"
]
}
]
},
...
]
}
}Though I think to make use of that, it seems there needs to be some changes made to the openedx-learning repo, namely extracting the logic out of the ObjectTagsByTaxonomySerializer and exposing a python API that can be used in this repo. Since currently It is only available through the rest api.
Any thoughts on this?
There was a problem hiding this comment.
My thinking was that it's OK to have this duplicated here, because eventually this will be re-implemented in the course-authoring MFE and fetch data using REST (that's out of scope for us now, but at some point ALL of these Studio pages will be using the new MFE only). When that happens, it can use the logic in openedx-learning.
We should definitely put in a comment stating that when this is moved into an MFE it should be simplified to use the REST API which already provides this grouping + sorting logic. But for now since it's already implemented this way, I think it's fine to leave it like this and get it done sooner.
There was a problem hiding this comment.
Ok @bradenmacdonald, that makes sense 👍
@yusuf-musleh Adding another comment, In addition to grouping by taxonomy, it is also necessary to group by parents, to be able to assemble each level of the component.
There was a problem hiding this comment.
@bradenmacdonald I see, yup, that make sense to me as well!
@ChrisChV Sounds good!
@yusuf-musleh OMG! Sorry! Yes, I completely forgot to add that the commands must be executed. Thanks, I will add it now. |
|
@yusuf-musleh @bradenmacdonald One thing I forgot to mention, The example shows disabled taxonomies. get_ojbect_tags returns object tags with disabled taxonomies. I think that's not the expected behavior, right? |
|
@ChrisChV For now we are not going to support disabled taxonomies, so it doesn't really matter. But I think that once the taxonomy is disabled, its tags should not be returned by get_object_tags. |
|
Sandbox deployment started. |
|
Sandbox deployment successful. Sandbox LMS is available at pr-33645-139931.staging.do.opencraft.hosting |
Thanks @ChrisChV ! |
|
Is this PR ready for a final review from @yusuf-musleh ? Then please ping me for the second review once it's approved, and I'll give a it a Core Contributor review. |
|
@bradenmacdonald It's ready for another review round |
|
@ChrisChV That looks great to me now! Just one very minor issue that's hopefully easy to fix and then I'll merge this: When I put my cursor in this area, the tag changes blue which makes it seems like I can click. But clicking it does not open the tag. Only clicking on the text does. Can you please change it so that clicking here will expand the tag? (Or so that hovering here doesn't turn it blue, but I think that's not as good.) CC @ali-hugo
|
@bradenmacdonald Done aa12f5c |
There was a problem hiding this comment.
Ah, sorry @ChrisChV I was just going to approve this but when I went through my checklist, I noticed an a11y issue. I can't open this with the keyboard.
When I tab/focus onto this "Discard changes" link and press TAB, I expect it to highlight "Tags 10" and let me press enter to reveal the tags. But instead it skips over it and goes to the bottom of the page ("Looking for help with Studio?").
So the .wrapper-tag-header needs to have tabindex=0. Also, from reading this page we need a few more a11y tweaks:
role=buttonon the.wrapper-tag-header- The
.wrapper-tag-headershould havearia-expanded=falsewhen tags are collapsed andaria-expanded=truewhen the tags are shown. - The
.wrapper-tag-headershould have anaria-controlsHTML attribute set to the ID of the content DIV (.wrapper-tag-content- you'll need to give it an ID)
Likewise, since the tags in the tree are interactive you should be able to TAB onto them and press ENTER to expand/collapse them.
Sorry for not catching this earlier; I'd prefer to give you everything in a single round of feedback.
|
Sandbox update request received. Deployment will start soon. |
|
Sandbox deployment started. |
|
Sandbox deployment successful. Sandbox LMS is available at pr-33645-139931.staging.do.opencraft.hosting |
|
@bradenmacdonald It's ready for another round |
|
@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. |
|
Sandbox update request received. Deployment will start soon. |
|
Hello! We think this may have broken a recent 2u build so we're going to revert and check some size issues. |
|
@ChrisChV No need to do anything re the above message ^. We don't think there's any issue with your code but 2U will need to revert the PR and re-merge it later. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
|
Update: I think there actually may have been a bug. We are getting |
|
Ah yeah @rgraber I've seen this before. Some part of the asset compilation uses @ChrisChV can you please re-submit, without the trailing comma in this line: |
Description
This adds:
Both are under the
new_studio_mfe.use_tagging_taxonomy_list_pageflag.Supporting information
Testing instructions
new_studio_mfe.use_tagging_taxonomy_list_pageflag on studio.paver update_assets. You need to clear the cache of your browser or use an incognito sessionnew_studio_mfe.use_tagging_taxonomy_list_pageflag on studio.Additional info
TODO