feat: Add v2 lib components content tags support - #771
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. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #771 +/- ##
=======================================
Coverage 89.62% 89.62%
=======================================
Files 499 499
Lines 8085 8089 +4
Branches 1705 1708 +3
=======================================
+ Hits 7246 7250 +4
Misses 812 812
Partials 27 27 ☔ View full report in Codecov by Sentry. |
34c4a8b to
ef06857
Compare
rpenido
left a comment
rpenido
left a comment
There was a problem hiding this comment.
LGTM 👍
- I tested this following the instructions from openedx-unsupported/frontend-app-library-authoring#400
- I read through the code
-
I checked for accessibility issues - Includes documentation
ef06857 to
cfef80e
Compare
|
@xitij2000 This is ready for CC review. |
xitij2000
left a comment
xitij2000
left a comment
There was a problem hiding this comment.
This looks good. One small minor suggestion.
| export const getContentDataApiUrl = (contentId) => ( | ||
| // Check if the contentId belongs to a V2 library component | ||
| contentId.startsWith('lb:') | ||
| ? new URL(`/api/libraries/v2/blocks/${contentId}/`, getApiBaseUrl()).href | ||
| : new URL(`/xblock/outline/${contentId}`, getApiBaseUrl()).href | ||
| ); |
There was a problem hiding this comment.
This is not a huge deal, but I feel it might be better to keep the URL function "dumb" and keep this if elsewhere.
i.e. have two function getXBlockContentDataApiURL and getLibraryContentDataApiUrl and the decision of which to be used can be elsewhere.
Does that make sense?
There was a problem hiding this comment.
Good point, yup I agree it would be cleaner to remove the logic from the URL function and keep it simple. I've updated it with your suggestion and put the logic where the URL functions are called in getContentData.
cfef80e to
c145b9f
Compare
c145b9f to
6ed9650
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. |
4 participants
Description
Add support for fetching content data for Library V2 components in content tags drawer.
Supporting Information
Related Tickets:
Testing Instructions
Private-ref: FAL-3599