feat: [FC-86] Home page course list section #18
Conversation
|
Thanks for the pull request, @PKulkoRaccoonGang! 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
PKulkoRaccoonGang
changed the title
PKulkoRaccoonGang
force-pushed
the
Peter_Kulko/courses-list-for-home-page
branch
from
d1b90de to
e1921f1
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18 +/- ##
==========================================
+ Coverage 96.41% 96.90% +0.48%
==========================================
Files 36 42 +6
Lines 251 323 +72
Branches 32 50 +18
==========================================
+ Hits 242 313 +71
- Misses 9 10 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
PKulkoRaccoonGang
force-pushed
the
Peter_Kulko/courses-list-for-home-page
branch
3 times, most recently
from
9bd7660 to
4a098d8
Compare
PKulkoRaccoonGang
requested review from
brian-smith-tcril,
diana-villalvazo-wgu and
ihor-romaniuk
src/data/course-discovery/api.ts
Outdated
| page_size: pageSize, | ||
| page_index: pageIndex, | ||
| }); | ||
| .post(getCourseDiscoveryUrl(), formData); |
There was a problem hiding this comment.
[inform/question]: During testing, I realized that the course_discovery API only returns courses for which enrollment_end is either not specified or the enrollment date for the course has not yet expired. We will address this issue on the backend side.
But, it seems strange to me that the legacy page displayed courses for which the enrollment date had passed and a student (or new student), upon seeing this course, would not be able to register for it.
Do you have any thoughts on this?
brian-smith-tcril
left a comment
brian-smith-tcril
left a comment
There was a problem hiding this comment.
Overall this looks great!
I left a couple comments about the loading/error/non-browsable states, but nothing that feels like it'll require major effort to resolve.
I also had some thoughts about the plugin slots being added that feel out of scope for this PR but should be addressed.
Things that make sense to address in a follow-up issue:
pluginPropsfor theHomeCourseCardSlotandHomeCoursesListSlot- I think this is more important for
HomeCourseCardSlotthanHomeCoursesListSlot. If a site operator is fully replacing the list they are more likely to have ways to fetch the data, but if they are just replacing the cards they'll likely want to use the course info associated with each card that is being replaced. - I think this makes sense as a follow-up issue (to be completed as part of this FC) instead of blocking this PR because:
- This PR satisfies the "has slots" requirement
- Updating the slots to include relevant
pluginPropsmakes sense to me as a standalone PR
- I think this is more important for
|
|
||
| if (isCoursesLoading) { | ||
| return ( | ||
| <Loading /> |
There was a problem hiding this comment.
I think the vh-100 in the Loading component is causing issues.
The spinner appears at the bottom of the screen at a standard window height for me
and it if off screen entirely at smaller heights
Screencast.From.2025-09-11.13-23-41.mp4
There was a problem hiding this comment.
Maybe it'd be worth considering using the Paragon card loading state here? That might be complicated because we don't know how many courses to expect (so we don't know how many cards to render), but it might still be worth looking into.
There was a problem hiding this comment.
The previous loading state for course list section has been updated to use the Paragon card loading state.
Since we cannot predict the exact number of courses, I suggest using getConfig().HOMEPAGE_COURSE_MAX || DEFAULT_COURSES_COUNT to determine how many skeleton cards to display (by default, this would be 9 cards).
Overall, I like how the loading appears using the card loading state. What are your thoughts?
Added in 2a50033
Example with Slow 4G (slow loading speed)
Screen.Recording.Sept.15.2025.from.Video.Trimmer.mp4
There was a problem hiding this comment.
I agree using the card loading state here looks great!
I am a bit curious about how plugin authors/site operators will feel about needing to handle the isLoading state in their CourseCard replacements if they aren't replacing CoursesList, but I can already think of a non-breaking way to address one of the concerns I'm foreseeing:
- "I want to replace the loaded
CourseCardcomponent. I don't want to implement a loading state for my replacement. I don't want to replace theCoursesListcomponent. I want to show one loading icon instead of one per card."- We could wrap the contents of the
Containerbeing returned in theif (isCoursesLoading)block with a slot that can be used for a generic loading state.
- We could wrap the contents of the
There was a problem hiding this comment.
In my local testing this feels a little "jumpy" with a fast connection
Screencast.From.2025-09-15.10-42-39.mp4
I expect this won't be an issue with sites that have 9 or more courses displayed on the home page. I also think part of what is making it feel "jumpy" to me is the footer moving (which feels like the same issue as #20)
There was a problem hiding this comment.
I am a bit curious about how plugin authors/site operators will feel about needing to handle the isLoading state in their CourseCard replacements if they aren't replacing CoursesList
On the one hand, without a generic loader slot, operators need to handle isLoading for each individual course card. On the other hand, by introducing a generic loader slot, we can give operators the option to provide a custom loader for the entire course list section if they replace course cards through slots.
At first glance, these two approaches may look similar, but adding a custom loader for the course list section seems simpler than implementing a custom skeleton (or another solution) for each individual card.
The only downside I see with this approach is that operators will need to take extra steps if they only want to replace the course card itself. However, this feels like a logical and very flexible solution 💯
Added in: dacb685
I also think part of what is making it feel "jumpy" to me is the footer moving (which feels like the same issue as #20)
This non-pinned footer causes certain issues, but I think that even with a pinned footer, we would still see minor layout "jump". In the future, when the footer has the correct position, it will look better, but the key issue is that if there is a significant difference between the value of HOMEPAGE_COURSE_MAX (or DEFAULT_COURSES_COUNT) and the actual number of courses on the page, the number of loading skeletons displayed will not match the real number of course cards.
For example, if HOMEPAGE_COURSE_MAX is set to 9, we will show 9 skeleton cards, but if there are only 3 courses in total, the final result will be 3 cards.
I tried porting the footer changes from the Learning MFE and checking how the course card loading looks with a pinned footer. It definitely looks better, but it doesn’t fully solve the problem of the mismatch between skeleton cards and the actual number of course cards.
Screen.Recording.Sept.16.2025.from.Video.Trimmer.mp4
That said, I agree with your point that this issue is most noticeable when the platform has only a small number of courses, which I believe is not a common case.
PKulkoRaccoonGang
force-pushed
the
Peter_Kulko/courses-list-for-home-page
branch
from
2693b29 to
2a50033
Compare
@brian-smith-tcril I don't mind doing this in a separate PR (added the appropriate TODO for future changes) Additionally, the latest updates include improvements to the course cards’ loading state, the Badge for the organization name, and a few minor enhancements. Please take a look when you have a chance. |
|
Overall, everything looks great in my local testing! Are the dependencies on the checklist in the PR description up to date?
That looks great! It might also be worth making a GH issue to track that. |
Created new issue: #22
Yes, this PR is currently awaiting review completion for two backend PRs: |
|
Thanks @PKulkoRaccoonGang! Overall this PR LGTM! I'm going to hold off on merging it until the 2 backend PRs are merged and I've had a chance to test this against the latest upstream versions of |
|
Since the edx-platform PRs have been merged, should this PR be updated to resolve conflicts and ensure things are at least working with edx-platform changes? |
PKulkoRaccoonGang
force-pushed
the
Peter_Kulko/courses-list-for-home-page
branch
from
dacb685 to
a684fdb
Compare
|
LGTM! The changes in b5345cf look great, and I've verified this works when running against the latest |
5 participants
Note
Dependencies:
Dependent PRs:
Warning
Currently, the list of courses returned by the API
/search/v1/course_discovery/does not contain courses for which the enrollment date has expired (enrollment_end). We plan to discuss this during a code review on the backend side.Description
This PR introduces several improvements related to the course listing on the Home page and prepares the foundation for upcoming changes on the Catalog page. It also includes enhancements to the CourseCard component, API integration updates, and minor UI improvements.
course-discoveryAPI has been moved to the application level (this API will also be used on Catalog page)getCourseDiscoveryUrlchanged from/search/course_discovery/to/search/v1/course_discovery/(updated version of existing API)Useful information
Legacy templates
Figma design
Initial setup
How Has This Been Tested?
http://apps.local.openedx.io:1998/catalog/).COURSES_ARE_BROWSABLEflag is false, the course list is not displayed.http://apps.local.openedx.io:1998/catalog/courses/<course_id>/about) without full page reloading (React Router).HOMEPAGE_COURSE_MAX(Django site configurations) value, or9, the button "View all courses" is displayed.