-
Notifications
You must be signed in to change notification settings - Fork 25
feat: add export library tags menu [FC-0049] #412
feat: add export library tags menu [FC-0049] #412
Conversation
Thanks for the pull request, @rpenido! 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. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #412 +/- ##
=======================================
Coverage 53.14% 53.14%
=======================================
Files 80 80
Lines 2021 2021
Branches 368 368
=======================================
Hits 1074 1074
Misses 914 914
Partials 33 33 ☔ View full report in Codecov by Sentry. |
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.
@rpenido I have left a question about the ENABLE_TAGGING_TAXONOMY_PAGES
env flag, but this change is working as expected 👍
- I tested this in my devstack using the PR test instructions.
- I read through the code
- I checked for accessibility issues by using my keyboard to navigate
-
Includes documentationN/A - User-facing strings are extracted for translation
const loadingStatus = LOADING_STATUS.LOADED; | ||
setConfig({ | ||
...getConfig(), | ||
ENABLE_TAGGING_TAXONOMY_PAGES: 'true', |
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.
We haven't defined this ENABLE_TAGGING_TAXONOMY_PAGES
env variable in this MFE yet.. I don't know if we need one.
@bradenmacdonald do we need a config flag to gate this feature here like we did for course authoring? #400 was approved by upstream without one.
@rpenido If we do need it, it needs to be added to the various .env
files, src/index.jsx
, and to the README.
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.
This MFE is not part of the standard install (yet) right? I don't think we need the flag here; it can just always be on.
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.
Done: b7af486
c4301da
to
b7af486
Compare
@rpenido @pomegranited It looks like these changes are ready for upstream review. I'm changing status on the Contributions board accordingly, but let me know if you have any objections. @navinkarkera Could you give this a maintainer review? |
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.
@rpenido Looks good to me. Please update the status and we can merge this openedx/edx-platform#34246 is merged.
Hi @navinkarkera! Thank you for the review. |
@rpenido 🎉 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
This PR adds an item in the
Settings
menu to allow a call of the function that exports the library tags to a CSV file.Supporting Information
Testing instructions
Manage Tags
optionExport Tags
menu from the detail page of an existing libraryPrivate-ref: