Tagging UX refinement v2 [FC-0036] - #34059
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. |
yusuf-musleh
left a comment
There was a problem hiding this comment.
👍 The updated implementation works quite well, just had a suggestion about the messaging format in the related (openedx/frontend-app-authoring#800) for the close message, which would need a small change here as well.
Great work @ChrisChV ! It's great seeing how all of this is coming together.
- I tested this: I confirmed that the dark background color has changed to the lighter one. I also tested that when you open the manage tags drawer for a unit from the course outline, add/remove tags updates automatically on the course outline page, including hiding/showing the tags count when 0 or more that 0 tags applied. I also tested the same for opening the tags drawer for a unit from inside the unit page, and the updated tags are reflected automatically in the TagList on the side of the page. And finally I tested that accessing the manage tags drawer from a component in a unit also reflects the changes on the page when adding/removing tags (including hiding and showing the tags count)
- I read through the code
- I checked for accessibility issues
- Includes documentation
-
I made sure any change in configuration variables is reflected in the corresponding client'sconfiguration-securerepository.
|
@ChrisChV There is some issue with the JS in this PR. When I have it checked out, I can't build the tutor image - it fails with some error in the Related, I'm seeing this error on the Unit page, even if I run the rebuilt assets command. |
There was a problem hiding this comment.
Ah, these import and export statements are probably the problem - I don't think the build process used in edx-platform supports them. But it could be something else - not sure.
There was a problem hiding this comment.
There are other factories that has the same syntax. My tutor is unconfigured, I'm going to try to configure it to try another syntax
There was a problem hiding this comment.
That error happened to me, but it was fixed with this line. Maybe clearing the cache?
There was a problem hiding this comment.
Nevermind, I found the r.js optimizer error
There was a problem hiding this comment.
Thx, let me know when you've found a fix.
There was a problem hiding this comment.
@bradenmacdonald I fixed the bug. It was an extrra , here You can test it? I still need to configure my tutor with the MFE
834d7ce to
876f61f
Compare
chore: Test to fix javascript error chore: Test to fix javascript error chore: Test to fix javascript error
21cdab6 to
61bb176
Compare
|
Hmm, I can see the lighter color, and now the JS build error is resolved, but the auto-refresh isn't working for me. I see that Course-Authoring sends the signals, but the CMS is not refreshing anything. @yusuf-musleh could you please confirm it's working for you in the latest version? If so, I'm good to merge this as-is. We're actually going to shift focus to the new MFE versions of these pages very soon, so I don't want to spend too much more time on the "legacy" views. |
|
@bradenmacdonald @ChrisChV I tried it out with the latest changes on this PR and the mfe PR, and ran through all the cases and can confirm that they're all working as expected on my devstack. |
|
@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. |
|
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. |
* style: drawer-cover color updated for all tagging drawers * feat: Update TagList component when a tag is updated on Manage tags drawer * feat: Refactor TagCount to be able to refresh the count * feat: Sync tag count in units

Description
This PR fixes the following issues:
Supporting information
Internal ticket: FAL-3601
Github issue: openedx/modular-learning#167
Related PR: openedx/frontend-app-authoring#800
Testing instructions
Enable
contentstore.enable_copy_paste_unitsandnew_studio_mfe.use_tagging_taxonomy_list_page flags.Before to all you need to run the taxonomy-sample-data.
1. the layer over the course outline is too dark when the tag drawer is open.
2. Refresh the count of tags on the unit/outline page(s) after the user makes edits in the tag drawer