feat: more customisability for course outline sidebar, course tabs #1908
feat: more customisability for course outline sidebar, course tabs #1908xitij2000 wants to merge 8 commits into
Conversation
|
Thanks for the pull request, @xitij2000! 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. |
b7261bd to
801197c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1908 +/- ##
==========================================
+ Coverage 91.30% 91.33% +0.03%
==========================================
Files 344 353 +9
Lines 5774 5877 +103
Branches 1351 1401 +50
==========================================
+ Hits 5272 5368 +96
- Misses 483 490 +7
Partials 19 19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| const CourseTabsNavigation = ({ | ||
| activeTabSlug = undefined, | ||
| className = null, |
There was a problem hiding this comment.
This is only used in one place and it doesn't send this param, so it can be removed.
There was a problem hiding this comment.
@brian-smith-tcril Here is the PR I mentioned during the discussion on the conference talk. There may still be changes based on how well it's working out.
musaabhasan
left a comment
There was a problem hiding this comment.
One small documentation/runtime mismatch stood out while reviewing the new plugin slot surface. It is worth fixing before the PR leaves draft because downstream plugin authors are likely to copy the slot ID directly from the README.
| @@ -0,0 +1,38 @@ | |||
| # Course Tab Navigation Slot | |||
|
|
|||
| ### Slot ID: `org.openedx.frontend.learning.course_tab_navigation.v1` | |||
There was a problem hiding this comment.
This documented slot ID does not match the implementation in CourseTabsNavigationSlot/index.tsx, which registers org.openedx.frontend.learning.course_tabs_navigation.v1 with tabs plural. Any env.config.jsx copied from this README would silently target the wrong slot and fail to customize the course tab navigation. Please align the README header and example key with the implemented ID, or rename the implementation if the singular ID is intended.
There was a problem hiding this comment.
Thanks! I've fixed this issue.
966ee19 to
1b58ad3
Compare
3caad2f to
864ae87
Compare
farhaanbukhsh
left a comment
There was a problem hiding this comment.
@xitij2000 Thank you for the amazing work. I have highlighted few nits also please fix the CI. I have done a pass on the code and it looks good. Will test it once.
| ## Example | ||
|
|
||
| ### Replaced with a custom component | ||
|  |
There was a problem hiding this comment.
please highlight the slot in the screenshot
There was a problem hiding this comment.
I am not sure what you mean by highlight. Adding any visual overlay would be confusing since you don't know if it's part of the UI change or applied on top.
There was a problem hiding this comment.
something like a red square pointing out what exactly is the slot 😅
There was a problem hiding this comment.
That could also imply that the slot is applying that style.
I'm updating the image by showing a before and after so that the difference is highlighted by contrast.
a9cf3b2 to
7b12566
Compare
| border: 1px solid #d7d3d1; | ||
| border-width: var(--learning-sidebar-outline-heading-wrapper-border-width, 1px); | ||
| border-style: solid; | ||
| border-color: var(--learning-sidebar-outline-heading-wrapper-border-color, var(--pgn-color-light-700)); |
There was a problem hiding this comment.
See discussion in the forums about this (and the other) variables proposed here:
farhaanbukhsh
left a comment
There was a problem hiding this comment.
This looks great, @xitij2000 I think what makes this PR stronger is adding testing instructions with a dummy env.config.jsx file.
Adds a new plugin slot (`CourseTabsNavigationSlot`) to enable customization, modification, or hiding of the course tab navigation. Updated relevant components to integrate with the new slot.
Splits `useCourseOutlineSidebar` into `useCourseOutlineData` and `useCourseOutlineSidebar` for improved separation of responsibilities. Updates components and styles to reflect this change. This change allows the usage of the course outline outside of a sidebar context.
…ce completion icons Introduces `CourseOutlineSidebarSectionCompletionIconSlot` and `CourseOutlineSidebarSequenceCompletionIconSlot` to enable customization, modification, or replacement of completion icons in the course outline. Updated components to integrate with these new slots.
Introduces `CourseOutlineSidebarUnitIconSlot` to enable customization, modification, or replacement of unit icons in the course outline sidebar. Refactors `UnitIcon` to support TypeScript and updates related components to integrate with the new slot.
…ng customization Replaces hardcoded styles with CSS variables to enable flexible theming and dynamic styling of the course sidebar components. Adjusts related JSX and SCSS for consistency.
…r heading customization Introduces a new plugin slot (`CourseOutlineSidebarHeadingSlot`) to support customization of the course outline sidebar heading. Refactors existing header logic into a reusable `CourseOutlineHeading` component for better modularity and integration with the new slot.
eaff289 to
cf4bdc3
Compare
…vigation Introduces a new plugin slot (`SequenceBottomNavigationSlot`) for extending or replacing the default bottom navigation in course sequences. Updates `Sequence` component to integrate with this slot. Includes documentation and examples for plugin usage.
cf4bdc3 to
5db75dc
Compare
This PR does a couple of things: