Repository navigation
Conversation
|
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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #875 +/- ##
==========================================
+ Coverage 92.46% 93.22% +0.75%
==========================================
Files 151 169 +18
Lines 1288 1447 +159
Branches 269 315 +46
==========================================
+ Hits 1191 1349 +158
- Misses 92 93 +1
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
This is looking great @ChrisChV!
Thank you for your work!
- I tested this using the instructions from the PR
- I read through the code
- I checked for accessibility issues
- Includes documentation
One suggestion so we can get rid of the patch while developing:
- Put your dummy data as a mock file in the tests
- Then, we can already implement all tests for the feature right away (we don't need the backend for that)
- Create the api layer and, for now, return the mock data
- Update the hook to call the api, and probably the hook code will also be final
That way, when the backend ships, we just need to update the api layer and the already tests that we wrote will help us to check if everything is fine. Also, we likely will have the coverage already sorted out.
Does that make sense? If it does, I will apply the same pattern for the other features.
| categoryLabel={<FormattedMessage {...messages.courseBadge} />} | ||
| className="text-dark-900 bg-light-500" | ||
| /> | ||
| <CourseCardMenu cardId={cardId} /> |
There was a problem hiding this comment.
Not directly in your change, but the Dropdown.Toggle is not centered with the badge.
We can fix it with the following adding className="d-flex align-items-center" to it.
<Dropdown onToggle={handleToggleDropdown}>
<Dropdown.Toggle
id={`course-actions-dropdown-${cardId}`}
className="d-flex align-items-center"
as={IconButton}| .course-card-pathways { | ||
| .course-card-pathways-container { |
There was a problem hiding this comment.
To make the height strip be the same with or without the dropdown to show
more items
| .course-card-pathways { | |
| .course-card-pathways-container { | |
| .course-card-pathways { | |
| min-height: 3.5rem; | |
| .course-card-pathways-container { |
| </Stack> | ||
| ) | ||
| : ( | ||
| <h2 className="dashboard-title"> |
There was a problem hiding this comment.
They seem to have the same styles
| <h2 className="dashboard-title"> | |
| <h2 className="course-list-title"> |
If we want to use the dashboard-title, we can remove the course-list-title from the scss
| description: 'Course list heading', | ||
| }, | ||
| coursesTitle: { | ||
| id: 'dahsboard.coursesTitle', |
There was a problem hiding this comment.
nit: typo
| id: 'dahsboard.coursesTitle', | |
| id: 'dashboard.coursesTitle', |
| if (!useIsPathwayPilotUIEnabled()) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
What do you think about moving this check outside? (i.e CourseHeaderActions)
| if (!useIsPathwayPilotUIEnabled()) { | |
| return null; | |
| } |
The component doesn't have anything tied to Pathways and we can test it without adding the flag.
There was a problem hiding this comment.
It is related; this same badge is used on the Pathways cards, and using that card for courses only makes sense if another item exists—in this case, the pathways.
|
@rpenido Thanks for your review!
Edit: I've tried what you suggested in openedx/frontend-app-learning#2159, and it seems like a good approach to me. I updated that in 0cf1bf4 |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…th the mock pathways as test fixtures
|
Thanks @ChrisChV! LGTM 👍 In the Figma file we have two alternatives for the unenrollment dialog. We didn't change that, right? @cassiezamparini Could you check if we want to use another design? |
@rpenido No, I didn't see that change when I checked Figma, but I can implement it here. @cassiezamparini Is it something that was recently added? |
…over and align it to the end of the strip Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…_mocks__ - Add tests for ItemBadge, PathwaysPanelSlot and isValidCssColor, and for the pathway pilot UI in CourseCard and CoursesPanel. - Move the mock pathways from __fixtures__ to __mocks__ and leave every __mocks__ folder out of the coverage. - Leave the no-document branch of isValidCssColor out of the coverage. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@rpenido @ChrisChV We can use this design: BUT, we need to incorporate the new text:
Is that possible? The learner needs to understand the repercussions. |
@cassiezamparini It is possible, but we need to consider the scenario where a course belongs to multiple pathways—specifically, how to display the titles of those
|
|
Ah got it @ChrisChV. Would this work? "This course is part of 1 bootcamp and 1 tutorial. If you unenroll, you'll lose access to the course, and your progress in those learning experiences may be affected. If you enroll again, you may be placed in a different session, and your progress might not carry over." |
@cassiezamparini I'm a bit concerned about the translation of plurals in this modal. Categories are created by the admin, which means translations must also be managed by the admin; even without this modal, each category already needs A simpler option would be to not name the categories in this message, e.g. "This course is part of 2 learning experiences". That plural can be translated the usual way in the frontend, so admins wouldn't need to enter anything extra. We could switch to the per-category text later if needed. What do you think? @rpenido @Agrendalath has any i18n been seen in the architecture? |
|
@ChrisChV Hmmm, got you. I want to keep the category names in the modal somehow, because “bootcamps and tutorials” means something to learners and “learning experiences” doesn’t. If we leave out the count and reuse the existing plural label, would that avoid the extra plural forms for admins?
Would the above be workable for the pilot? Or what other ideas do you have? |
@ChrisChV There is a discussion here: TLDR: no translations for the pilot, but we will have the |
@rpenido Thanks!
Yes, I think it is possible. I will implement it this way. |
c47660d to
da9d579
Compare
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
Can you update the Testing instructions? We don't need the patch anymore, right?
|
@kdmccormick and @ormsbee, this is ready for review! |





Description
ENABLE_PATHWAY_PILOT_UIconfig flag and theuseIsPathwayPilotUIEnabledhook.pathwaysPanelslot.Included instrip to course cards with their pathways. Labels that don't fit go to a "+N" popover.Supporting Information
Testing Instructions
ENABLE_PATHWAY_PILOT_UIusing this plugin:Refineare attached to the top.Included inin some courses. Verify that you can see the hidden pathways by clicking the count button.Included inby resizing the screen.Notes