feat: [FC-86] created course list for Catalog page#5
Conversation
openedx-webhooks
added
the
open-source-contribution
|
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. |
PKulkoRaccoonGang
changed the title
PKulkoRaccoonGang
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5 +/- ##
==========================================
+ Coverage 89.09% 95.32% +6.23%
==========================================
Files 10 25 +15
Lines 55 171 +116
Branches 0 21 +21
==========================================
+ Hits 49 163 +114
- Misses 6 8 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
Adds reusable loading and course card components, implements the Course Catalog page layout, and wires up navigation and tests.
- Introduces
LoadingSpinnerand full-pageLoadingcomponents to display spinners with proper accessibility. - Adds
CourseCardcomponent (with utils, styles, and tests) to render course information and images. - Implements
AlertNotification, updates environment ports, and exports new generics for use in the Catalog page.
Reviewed Changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/generic/loading-spinner/types.ts | Defines spinner prop types |
| src/generic/loading-spinner/messages.ts | Adds i18n message for screen-reader text |
| src/generic/loading-spinner/index.tsx | Implements LoadingSpinner and Loading wrapper |
| src/generic/loading-spinner/LoadingSpinner.test.tsx | Tests spinner rendering, sizes, and accessibility |
| src/generic/index.ts | Exports new generic components |
| src/generic/index.scss | Imports styles for course-card and sub-header |
| src/generic/course-card/utils.ts | Builds full image URLs from LMS base |
| src/generic/course-card/types.ts | Declares course and card props types |
| src/generic/course-card/messages.ts | Defines start-date i18n message |
| src/generic/course-card/index.tsx | Renders course cards with images, titles, and dates |
| src/generic/course-card/constants.ts | Holds date formatting options |
| src/generic/course-card/CourseCard.test.tsx | Tests card rendering, links, and image sources |
| src/generic/course-card/CourseCard.scss | Styles course-card layout and responsive behavior |
| src/generic/alert-notification/types.ts | Declares alert notification prop types |
| src/generic/alert-notification/index.tsx | Implements AlertNotification with variant and icon |
| src/generic/alert-notification/AlertNotification.test.tsx | Tests alert rendering and variants |
| src/mocks/index.ts | Re-exports mock data |
| src/mocks/course.ts | Provides mockCourseResponse |
| .env.test | Updates test BASE_URL port |
| .env.development | Updates development PORT and BASE_URL |
Comments suppressed due to low confidence (4)
src/generic/course-card/index.tsx:24
- The route omits the
/catalogprefix used elsewhere (/catalog/courses/<id>/about). Consider updating to/catalog/courses/${course.id}/aboutto keep navigation consistent.
to={`/courses/${course.id}/about`}
src/generic/alert-notification/types.ts:2
- Using a general string for
variantreduces type safety. Consider restricting it to known alert variants (e.g., 'info' | 'warning' | 'success' | 'danger').
variant: string;
src/generic/loading-spinner/messages.ts:5
- [nitpick] The message id
'category.generic.loading'is inconsistent with other namespaces (e.g.,'generic.course-card.start-date'). Align ids to a common convention for clarity.
id: 'category.generic.loading',
src/generic/course-card/index.tsx:32
- There’s no test case covering the scenario when
orgImgis missing to ensure the fallback logo is rendered. Consider adding a test for that branch.
logoSrc={course.data.orgImg ? getFullImageUrl(course.data.orgImg) : undefined}
src/index.test.jsx
Outdated
| it('navigates to /courses route', () => { | ||
| renderWithProviders('/courses'); | ||
| // eslint-disable-next-line no-underscore-dangle | ||
| expect(window._testHistory).toContain('/courses'); |
There was a problem hiding this comment.
can we check courses content instead? it seems this test will never fail since is checking the path you are providing and setting inside renderWithProviders, with the routes you defined there, not really checking index.tsx routing
There was a problem hiding this comment.
Yes, that would be better. Corrected in b5055e1
| render, | ||
| renderHook, | ||
| within, | ||
| waitFor, |
There was a problem hiding this comment.
should we convert this into a ts file?
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 few comments - nothing super major, mostly questions about hardcoded spacing.
* feat: created course list for Catalog page
PKulkoRaccoonGang
force-pushed
the
Peter_Kulko/course-course-card-component
branch
from
a1ec006 to
d74d889
Compare
|
@brian-smith-tcril @diana-villalvazo-wgu I have rebased this PR and added the suggested changes after the review. Please take a look. |
brian-smith-tcril
left a comment
brian-smith-tcril
left a comment
There was a problem hiding this comment.
I pulled this down and was able to test it against tutor with a couple changes:
- Tutor doesn't know about this MFE so I just used port 2000 as a workaround, it'd be great to add a tutor plugin to the docs to avoid the need to do this
- Styles weren't loading at all because the version of
frontend-platformthis MFE is using doesn't include openedx/frontend-platform#806, so I needed to bump the version
I left some specific comments, but the more I look at this PR the more I feel there's a bigger picture question to ask: do we need override so much stuff?
A lot of the styles in this PR are overriding Paragon defaults - do we need to do that?
The example Card component from the "Card status" example on the Paragon docs site looks quite close to what the overrides are doing here, and using the design system as designed feels better to me than overriding things left and right.
I think using more "out of the box" Paragon components would lead to a much more maintainable MFE.
|
More context on the "big picture" question I raised here: From Slack (https://openedx.slack.com/archives/C08QR8K7K38/p1749830688812029):
|
|
@brian-smith-tcril thank you for bringing this to my attention 💯
I support using Paragon components "out of the box" without adding unnecessary styles (where possible). This is definitely a good approach! Removed all styles that were added to match the Figma layout.
Good point! I added a small installation guide using the draft PR for tutor-mfe, which adds the Catalog MFE. We have already tried deploying with the new Catalog MFE and it works.
I upgraded the frontend-platform to |
| expect(getByText(mockCourseResponse.data.content.displayName)).toBeInTheDocument(); | ||
| expect(getByText(mockCourseResponse.data.org)).toBeInTheDocument(); | ||
| expect(getByText('Starts: Apr 1, 2024')).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
Optional: RTL generally recommend using the screen helper, it's shorter and helps if you also appreciate the shorter syntax and less need to pick which selectors to define, so instead of
const { getByText } = renderComponent();
expect( getByText(...) ).toBlah()
they encourage to do:
import { ..., screen } from '../setupTest';
renderComponent();
expect( screen.getByText(...) ).toBlah()
Again this is optional, not a requirement.
There was a problem hiding this comment.
If RTL generally recommend, then I also support this idea 💯
Added screen for tests in this PR (other tests will be updated iteratively, when adding subsequent PRs)
brian-smith-tcril
left a comment
brian-smith-tcril
left a comment
There was a problem hiding this comment.
Overall this is looking great using default Paragon styles!
I left one small comment about the use of jpegs for the no course image.
When testing, it also felt to me like the margin between the course image and the card header was too big. This is a default paragon thing, so I created an issue to discuss it openedx/paragon#3660
src/assets/no-course-image.jpg
Outdated
There was a problem hiding this comment.
Would it be possible to use SVGs for this instead?
There was a problem hiding this comment.
Replaced, good idea
|
I played around with the card a bit more and I think the issue isn't so much that there's too much margin above the header, but that the card overall doesn't feel balanced. I tried moving the start date from the footer into a section as a quick test, and while it doesn't look perfect it does feel more balanced to me
diff --git a/src/generic/course-card/index.tsx b/src/generic/course-card/index.tsx
index 07fe49d..564f3ec 100644
--- a/src/generic/course-card/index.tsx
+++ b/src/generic/course-card/index.tsx
@@ -38,12 +38,13 @@ export const CourseCard = ({ course }: CourseCardProps) => {
subtitle={course.data.org}
/>
{formattedDate && (
- <Card.Footer className="justify-content-start">
+ <Card.Section className="justify-content-start">
{intl.formatMessage(messages.startDate, {
startDate: formattedDate,
})}
- </Card.Footer>
+ </Card.Section>
)}
+ <Card.Footer />
</Card>
);
};
|
|
Tried another thing
diff --git a/src/generic/course-card/index.tsx b/src/generic/course-card/index.tsx
index 07fe49d..20af8bf 100644
--- a/src/generic/course-card/index.tsx
+++ b/src/generic/course-card/index.tsx
@@ -35,15 +35,18 @@ export const CourseCard = ({ course }: CourseCardProps) => {
/>
<Card.Header
title={course.data.content.displayName}
- subtitle={course.data.org}
/>
- {formattedDate && (
- <Card.Footer className="justify-content-start">
+ <Card.Section>
+ <div>{course.data.org}</div>
+ {formattedDate && (
+ <div>
{intl.formatMessage(messages.startDate, {
startDate: formattedDate,
})}
- </Card.Footer>
- )}
+ </div>
+ )}
+ </Card.Section>
+ <Card.Footer />
</Card>
);
};I don't think either this or the previous diff I tried are good enough to ship, but it does seem like there are ways to make the card look nice without overriding Paragon styles. I think getting some some design input on this one would be helpful. |
|
@brian-smith-tcril thanks for looking into this 💯 Do I need to add the "workarounds" you suggested to improve the Paragon Card display? Or do we want to resolve this issue within Paragon, after getting feedback from the designer/others, without blocking this PR? |
|
@PKulkoRaccoonGang I think moving the start date out of the card footer should happen in this PR. Either of the diffs I commented earlier would be fine, but another option (that would likely provide even more flexibility) would be to not use a After that, we can create a new issue on this repo to determine a final design (ideally without needing to change Paragon). |
|
@brian-smith-tcril Thanks for clarification! Currently the card looks like this: 
Moved content from Card.Header, Card.Footer to Card.Section in this commit: e498645 Created a new issue for Determination the final design for the course Card component: #10 |
brian-smith-tcril
left a comment
brian-smith-tcril
left a comment
There was a problem hiding this comment.
I tested this locally and it's looking great! One last comment, but after that I think this should be good to merge!
brian-smith-tcril
left a comment
brian-smith-tcril
left a comment
There was a problem hiding this comment.
LGTM!
Thank you so much for working through the review process with me! I'm very happy with how this turned out!
7 participants
Note
Description
This PR includes the following changes:
setupTest.jsconfigurationInitial setup
How Has This Been Tested?
http://apps.local.openedx.io:1998/catalog/courses).http://apps.local.openedx.io:1998/catalog/courses/<course_id>/about) without full page reloading (React Router).Merge Checklist