-
Notifications
You must be signed in to change notification settings - Fork 10
feat: [FC-86] Added Course About sidebar #38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: [FC-86] Added Course About sidebar #38
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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #38 +/- ##
==========================================
+ Coverage 98.15% 98.57% +0.42%
==========================================
Files 73 89 +16
Lines 597 845 +248
Branches 79 136 +57
==========================================
+ Hits 586 833 +247
- Misses 11 12 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1c757fe to
053f10c
Compare
| @@ -0,0 +1,15 @@ | |||
| // TODO: Temporary placeholder for the course overview | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[question] You are planning to replace this placeholder with dynamic code in the next pull requests, aren't you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
src/utils.ts
Outdated
| */ | ||
| export const formatDate = (dateString: string): string => { | ||
| const date = new Date(dateString); | ||
| const languagePreference = getCookie(getConfig().LANGUAGE_PREFERENCE_COOKIE_NAME); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Looks good overall, but I’d suggest normalizing the locale before using it in toLocaleDateString.
Short tags like en, uk, or ar can lead to inconsistent formatting across browsers.
Consider mapping them to full locale codes (e.g., en-US, uk-UA, ar-SA) for more predictable output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replaced this function with intl.formatDate
refactor: some refactoring refactor: some refactoring test: added tests refactor: added plugin slots refactor: changed page layout test: added some tests refactor: some refactoring refactor: corrected formatDate function
24c85c4 to
7723355
Compare
| .course-about-intro-wrapper { | ||
| @media (--pgn-size-breakpoint-max-width-xl) { | ||
| .row { | ||
| flex-direction: column-reverse; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[inform]: According to the new changes, we no longer need column-reverse, column is enough.
| return ( | ||
| <Container className="p-3"> | ||
| {/* eslint-disable-next-line react/no-danger */} | ||
| <div dangerouslySetInnerHTML={{ __html: courseAboutData.aboutSidebarHtml }} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During testing, I noticed an issue that I’m not sure should be addressed within this MFE.
On the Schedule & Details page, when saving sidebar content, the following data is sent to the backend: about_sidebar_html: "<p>321</p>"
The sidebar on the legacy Course About page displays this as shown below (most likely because this field originally accepted plain text, and with the introduction of the WYSIWYG editor, the content is no longer rendered correctly):
In the new Catalog MFE, the Course About page renders the same content using dangerouslySetInnerHTML
The issue appears when there is no content in the WYSIWYG editor. In that case, the backend receives: about_sidebar_html: ""
However, the frontend still receives the following value for about_sidebar_html:
So technically, the field still contains data, even though there’s no actual content.
In upcoming PRs, new plugin-slots will be added for the Course About page.
I believe the functionality for adding sidebar content should exist exclusively through the plugin-slots. This would allow us to eventually remove both the use of dangerouslySetInnerHTML and the sidebar content editing functionality from the Schedule & Details page.
It might make sense to create a separate issue or DEPR to track this. Any thoughts about this?
|
|
||
| const SidebarDetailsItem = ({ icon, label, value }: SidebarDetailsItemProps) => ( | ||
| <> | ||
| <Stack className="justify-content-between flex-wrap p-3" gap={2} direction="horizontal"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
brian-smith-tcril
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is looking good!
In my local testing things worked quite well.
I left a few comments:
- One about about a way to move away from using some custom styles
- One with a question about Price
- A couple about the social links slot
Looking forward to hearing your thoughts!
a60b477 to
95640a3
Compare
brian-smith-tcril
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for working through the review process with me!
This looks great!

Note
Additional plugin slots and course overview content sections will be added in upcoming PRs.
Note
Created a DEPR: openedx/openedx-platform#37022 and a new PR in edx-platform for the
prerequisitesandocw_linkssections.Description
This PR introduces a sidebar to the Course About page.
Useful information
Initial setup
tutor-mfeplugin from the draft PR that adds support for Catalog MFE.How Has This Been Tested?
catalog/courses/<course_id>/aboutCourse About page