feat: course libraries page [FC-0076]#1641
Conversation
|
Thanks for the pull request, @navinkarkera! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
openedx-webhooks
added
the
open-source-contribution
navinkarkera
force-pushed
the
navin/fal-4006/course-libraries-page
branch
from
caea2df to
e9d41f5
Compare
navinkarkera
changed the title
pomegranited
left a comment
pomegranited
left a comment
There was a problem hiding this comment.
👍 Looks and works great @navinkarkera !
- I tested this using the PR test instructions.
- I read through the code
- I checked for accessibility issues by using my keyboard alone to navigate this new page.
- Includes documentation -- code comments for new apis/hooks.
- User-facing strings are extracted for translation
| const intl = useIntl(); | ||
| const componentIcon = getItemIcon(info.blockType); | ||
| const breadcrumbs = tail(info.breadcrumbs) as Array<{ displayName: string, usageKey: string }>; | ||
| const unitUsageKey = breadcrumbs[breadcrumbs.length - 1].usageKey; |
There was a problem hiding this comment.
[suggestion] I get an error on this line sometimes -- which was resolved by re-running reindex_studio.
But maybe check breadcrumbs.length before assuming there's a wrapper unit?
Breadcrumb.error.mp4
src/CourseAuthoringRoutes.jsx
Outdated
| import { DECODED_ROUTES } from './constants'; | ||
| import CourseChecklist from './course-checklist'; | ||
| import GroupConfigurations from './group-configurations'; | ||
| import CourseLibraries from './course-libraries/CourseLibraries'; |
There was a problem hiding this comment.
nit: worth importing this from an index file like the other course components?
| const componentIcon = getItemIcon(info.blockType); | ||
| const breadcrumbs = tail(info.breadcrumbs) as Array<{ displayName: string, usageKey: string }>; | ||
| const unitUsageKey = breadcrumbs[breadcrumbs.length - 1].usageKey; | ||
| const blockLink = `${getConfig().STUDIO_BASE_URL}/container/${unitUsageKey}`; |
There was a problem hiding this comment.
[suggestion] Is there a way to re-use the methods in search-modal.SearchResult like getUnitComponentUrlSuffix instead of setting the URL explicitly here?
There was a problem hiding this comment.
@pomegranited SearchResult component seems to generate new mfe links and ignores server waffle flags, so I did not reuse it. It is easier to always point to server url as it redirects to mfe if enabled else displays legacy unit page. Note that here we always open the link in a new tab so full refresh is inevitable (react-router fast routing won't help).
There was a problem hiding this comment.
Ah ok, that makes sense.
navinkarkera
force-pushed
the
navin/fal-4006/course-libraries-page
branch
2 times, most recently
from
f547160 to
87d7567
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1641 +/- ##
==========================================
+ Coverage 93.30% 93.33% +0.02%
==========================================
Files 1101 1107 +6
Lines 21855 22002 +147
Branches 4646 4768 +122
==========================================
+ Hits 20392 20535 +143
+ Misses 1398 1396 -2
- Partials 65 71 +6 ☔ View full report in Codecov by Sentry. |
ChrisChV
left a comment
ChrisChV
left a comment
There was a problem hiding this comment.
@navinkarkera Looks good! There are some nits
| readyToSync: boolean; | ||
| } | ||
|
|
||
| export const getEntityLinksByDownstreamContext = async ( |
There was a problem hiding this comment.
@navinkarkera Could you add a simple test to this function?
There was a problem hiding this comment.
Done. Added test case for the hook that covers this API as well. 20650d5
navinkarkera
force-pushed
the
navin/fal-4006/course-libraries-page
branch
from
87d7567 to
20650d5
Compare
ChrisChV
changed the title
4 participants
Description:
Adds Libraries page that lists all library components being used in the current course to
Content > Libraries.Part of: #1565
Depends on: openedx/openedx-platform#36190
Private-ref: FAL-4006Test instructions:
tutor images build openedx-dev && tutor dev launch -I --skip-build.Content > Librariesnavigation menu item.Library Contentpicker, copy & paste orProblem bankoption.Concerns:
Some of the requirements listed in the comment here: #1565 (comment) are not finalized.