Repository navigation
Conversation
Show, above the learning header, the pathways that include the current course and in which the learner is enrolled. The labels that do not fit are moved to a "+N" popover, following the same logic as the course card strip in the learner dashboard. - Add the `course-pathways` data layer: `getPathwaysByCourse` in the API layer, which returns an empty map until the backend exists, and the `useCoursePathways` hook, which extracts the course pathways from it. - Add the `CoursePathwaysStrip` and `MorePathwaysPopover` components and the `useVisiblePathways` hook to split the visible and hidden pathways. - Add the `org.openedx.frontend.learning.course_pathways_strip.v1` plugin slot, rendered by `TabPage` once the course page has loaded. - Add mock pathways as test fixtures.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s strip and request its pathways
|
Thanks for the pull request, @ChrisChV! 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. |
rpenido
left a comment
There was a problem hiding this comment.
LGTM 👍
Thank you for your work, @ChrisChV!
- I tested this using the instructions from the PR
- I read through the code
- I checked for accessibility issues
- Includes documentation
Note: I think the code here is missing the fix that you applied here: openedx/frontend-app-learner-dashboard@5a5fd87
What are your thoughts on creating a @openedx/frontend-component-pathways for our shared code?
This and openedx/frontend-app-learner-dashboard#875 share a lot of components and functions. Handling it in two places in the long term would be a pain.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2159 +/- ##
==========================================
+ Coverage 95.25% 96.20% +0.95%
==========================================
Files 372 376 +4
Lines 6053 6094 +41
Branches 1499 1445 -54
==========================================
+ Hits 5766 5863 +97
+ Misses 275 222 -53
+ Partials 12 9 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
It's in the code: d80d6e6 |
…over and align it to the end of the strip Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Hi @kdmccormick and @ormsbee! This is ready for review! |
| ENABLE_PATHWAY_PILOT_UI | ||
| Enables the pathways pilot UI: the strip above the course header that lists the pathways, in which | ||
| the learner is enrolled, that include the course. Disabled by default; set it to 'true' to enable it. | ||
| When disabled, the pathways are not requested and the ``org.openedx.frontend.learning.course_pathways_strip.v1`` | ||
| plugin slot is not rendered. | ||
|
|
There was a problem hiding this comment.
This flag seem redundant to me for two reasons:
- If operators don't want to see the pathway strip, then they shouldn't create pathways
- If operators want create pathways and still don't want the strip, they can override the plugin slot
In the spirit of reducing complexity, can we avoid adding this new config flag?
There was a problem hiding this comment.
Hi @kdmccormick!
The idea behind the feature flag is to allow us to merge some WIP code (like this one, missing the backend implementation) in main, instead of having a bunch of open PRs in a chain and/or conflicting with each other. We can will remove it once we have the full feature in place.
What do you think about that? Do you see a better way to handle it?
There was a problem hiding this comment.
Could you make the app gracefully degrade if the REST API is not available? i.e., it would try to hit the getPathwayByCourse API, see the 404, and then log a console warning that it cannot render the pathway strip.
There was a problem hiding this comment.
I removed the flag in b4369de. For now, there is no REST API call to degrade from: getPathwaysByCourse is a stub that returns an empty map until the backend endpoint exists, so the app never makes a request, and the strip is simply not rendered.
| courseCount: number; | ||
| category?: string; | ||
| categoryLabel?: string; | ||
| categoryBackgroundColor?: string; | ||
| categoryTextColor?: string; | ||
| }; | ||
| progress?: { | ||
| completedCourseCount: number; |
There was a problem hiding this comment.
Perhaps this discussion is better suited for the backend PRs, but these APIs must not assume that Pathways are built of courses. Pathways are built of Items, so we should have fields like itemCount and completedItemCount rather than courseCount and completedCourseCount
| @@ -0,0 +1,55 @@ | |||
| # Course Pathways Strip Slot | |||
|
|
|||
| ### Slot ID: `org.openedx.frontend.learning.course_pathways_strip.v1` | |||
There was a problem hiding this comment.
I am somewhat inclined to stay that these strips could have other uses beyond pathways-- things like Your trial course access ends in 6 days or You are viewing an archived run of this course. To earn credit, view this new run <link>.
So maybe the Pathways info should just be the default content of a new generic org.openedx.frontend.learning.course_info_strip.v1 slot?
But I can also see the counterargument: if the slot is too generic, it will get misused and/or it becomes harder to understand. The org.openedx.frontend.learning.course_pathways_strip.v1 has an obvious purpose, and that's a good thing.
@brian-smith-tcril , could I get your opinion on this?
There was a problem hiding this comment.
I am somewhat inclined to stay that these strips could have other uses beyond pathways
100% agree.
Having the pathways strip as the default content in a org.openedx.frontend.learning.course_info_strip.v1 slot would definitely make sense to me!
There was a problem hiding this comment.
actually, before you make this change @ChrisChV , Brian highlighted a deeper issue: frontend-base assumes that the header is the topmost element and the footer is the bottom-most element, with app content all sandwiched between those. So, if we made the plugin slot part of frontend-app-learning, then we would immediately hit an issue when trying to migrate this repo to frontend-base (which is happening in parallel as we speak).
the fix would be to make a generic info-strip slot within the header itself, and have frontend-app-learning push the pathways info into that slot (feel free to chime in Brian if I'm describing that wrong).
that would be a bit more work, so I think we should check with product about whether it's important that this strip must be above the course header, or whether it could potentially be below the course header instead, which would avoid the whole issue. I'll ask in Cassie in Slack when I'm back in office Tuesday morning, or if you're working Monday feel free to ask her.
There was a problem hiding this comment.
the fix would be to make a generic info-strip slot within the header itself, and have frontend-app-learning push the pathways info into that slot
That's the gist of it.
In frontend-base we'd need to make it so an app can "put something into" a new info strip slot (as opposed to having the app directly render it).
Replicating the "put something into" in frontend-platform/frontend-build/frontend-plugin-framework land would be a little more complicated (but doable). The first idea that comes to mind would be to have the slot live in frontend-component-header, and have the slot render default content that is passed in as a prop from the consuming application (so every app other than frontend-app-learning just doesn't pass anything and the slot is empty, but frontend-app-learning passes the pathways strip.
There was a problem hiding this comment.
that would be a bit more work, so I think we should check with product about whether it's important that this strip must be above the course header, or whether it could potentially be below the course header instead, which would avoid the whole issue. I'll ask in Cassie in Slack when I'm back in office Tuesday morning, or if you're working Monday feel free to ask her.
@kdmccormick @ChrisChV What exactly constitutes the course header: the course title only, or course title + course navigation?
@ali-hugo and I had a look, and think it could work if it's just below the course title, i.e:
FYI: I'm on leave from tomorrow and will be back on Monday.
There was a problem hiding this comment.
^ that works great, thanks @cassiezamparini !
@ChrisChV , could we go with a org.openedx.frontend.learning.course_info_strip.v1 below the course header?
| // TODO The backend is missing. | ||
| // This endpoint should receive a list of course IDs and return a map | ||
| // containing the pathways in which the learner is enrolled for each course. | ||
| // The response must be mapped here to the `PathwayData` shape. | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| export const getPathwaysByCourse = async (_courseIds: string[]): Promise<PathwaysByCourse> => ({}); |
There was a problem hiding this comment.
when there is a version of the backend ready, could you try making a sandbox for this PR by adding the create-sandbox label using the PR description to point it at the backend branch? https://github.com/open-craft/pr-sandbox-automation/tree/main/docs#custom-configuration
There was a problem hiding this comment.
I started a PR for a sandbox for the whole pathways here: openedx/openedx-platform#39205
The blocker right now is that we need a patch to apply the mock with dummy data to actually test it.
Description
CoursePathwaysStripSlotSupporting information
Testing instructions
ENABLE_PATHWAY_PILOT_UIflagNotes