-
Notifications
You must be signed in to change notification settings - Fork 80
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-0044] Course unit page - Display sidebar component #832
feat: [FC-0044] Course unit page - Display sidebar component #832
Conversation
Thanks for the pull request, @PKulkoRaccoonGang! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #832 +/- ##
==========================================
+ Coverage 90.94% 91.14% +0.20%
==========================================
Files 520 542 +22
Lines 9157 9479 +322
Branches 1920 1984 +64
==========================================
+ Hits 8328 8640 +312
- Misses 797 806 +9
- Partials 32 33 +1 ☔ View full report in Codecov by Sentry. |
89db038
to
38f42b6
Compare
* feat: added Sidebar with unit info * feat: added unit location * refactor: added legacy behavior * feat: added live variant * refactor: code refactoring * feat: added tests and translations * feat: added new font size * refactor: after review
38f42b6
to
55a9888
Compare
* feat: [AXIMST-24] sidebar buttons functional * refactor: removed modal extra className * refactor: refactoring after review
…changes (#135) * feat: [AXIMST-25] added alert notification about unpublished changes * feat: added tests * feat: added translations
Sandbox deployment successful 🚀 |
Sandbox deployment failed 💥 |
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.
The code looks good for the most part, but I do have some requests for changes. Thanks!
return ( | ||
<span className="course-unit-sidebar-date-and-with"> | ||
<h6 className="course-unit-sidebar-date-timestamp m-0 d-inline"> | ||
{releaseInfo.releaseDate} |
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.
Do we know if the dates get localized properly in the backend?
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.
The date in the backend is always in UTC. If we want it to be in the users time zone we can use <FormattedDate />
and <FormattedTime />
from i18n
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, you are right 💯
I had internal communication on this issue, now we have repeated legacy behavior. Let's take these improvements related to internationalization out of scope.
Sandbox deployment successful 🚀 |
src/course-unit/sidebar/index.jsx
Outdated
import useCourseUnitData from './hooks'; | ||
import messages from './messages'; | ||
|
||
const Sidebar = ({ blockId, isDisplayUnitLocation, ...props }) => { |
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.
@PKulkoRaccoonGang I'm working on a sidebar for tags that uses your sidebar as a base. A discussion has arisen about how to make the sidebar styles reusable, perhaps that the sidebar component is a wrapper for its content (until now we would have three sidebars). What do you think?
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 think this is a good idea 👍
Do you plan to split the sidebar presented in this PR into a wrapper and sidebar content?
Sandbox deployment successful 🚀 |
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.
Looks good to me, provided we deal with internationalization separately.
@PKulkoRaccoonGang 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Settings
Description
This pull request added a sidebar for the Course unit page and an alert notifying about unpublished changes.
The primary features were implemented:
Issue: openedx/platform-roadmap#321
Developer notes
Design
Figma design
Testing instructions
contentstore.new_studio_mfe.use_new_unit_page
in the CMS admin panel.ENABLE_UNIT_PAGE=true
is enabled.Screen.Recording.2024-02-29.at.17.01.00.mov