-
Notifications
You must be signed in to change notification settings - Fork 75
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: Created Course updates page #581
feat: Created Course updates page #581
Conversation
vladislavkeblysh
commented
Aug 29, 2023
- Created Course updates page
- Ability to add new course update and handouts, edit, delete updates, edit handouts
Thanks for the pull request, @vladislavkeblysh! 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 ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #581 +/- ##
==========================================
+ Coverage 84.54% 84.62% +0.08%
==========================================
Files 279 301 +22
Lines 4582 4847 +265
Branches 1029 1072 +43
==========================================
+ Hits 3874 4102 +228
- Misses 684 721 +37
Partials 24 24
☔ View full report in Codecov by Sentry. |
3861234
to
8a8d2a1
Compare
516afdb
to
6c13a0b
Compare
Hi @openedx/teaching-and-learning! Would someone be able to please review/merge this for us? Thank you! |
<Badge | ||
className={classNames('processing-notification', { | ||
'is-show': isShow, | ||
})} | ||
variant="secondary" | ||
aria-hidden={isShow} | ||
> | ||
<Icon className="processing-notification-icon" src={IconSettings} /> | ||
<h2 className="processing-notification-title"> | ||
{capitalize(title)} | ||
</h2> | ||
</Badge> |
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.
Is there a reason you decided to use the Badge
component instead of the Toast
component?
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.
We tried to replicate the behavior of the legacy as closely as possible. This component should appear on the right (while the Toast appears on the left) and should visually resemble the legacy as much as possible. Additionally, the Toast has a close button and slightly different functionality
src/generic/WysiwygEditor.jsx
Outdated
@@ -73,7 +73,7 @@ export const WysiwygEditor = ({ | |||
minHeight={minHeight} | |||
editorContentHtml={initialValue} | |||
setEditorRef={setEditorRef} | |||
onChange={handleUpdate} | |||
updateContent={handleUpdate} |
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.
updateContent={handleUpdate} | |
onChange={handleUpdate} |
This needs to be onChange. The prop has been renamed in frontend-lib-content-components
because of a inherited prop conflict.
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.
Fixed
dispatch(updateSavingStatuses({ createCourseUpdateQuery: RequestStatus.PENDING })); | ||
dispatch(showProcessingNotification(NOTIFICATION_MESSAGES.saving)); | ||
const courseHandouts = await editHandouts(courseId, data); | ||
dispatch(editCourseHandouts(courseHandouts)); |
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.
On edit, the api call is working as expected, but the data does not include the updates made to update. This is probably connected to the change that you made in the wysiwyg editor.
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.
Fixed
dispatch(updateSavingStatuses({ createCourseUpdateQuery: RequestStatus.PENDING })); | ||
dispatch(showProcessingNotification(NOTIFICATION_MESSAGES.saving)); | ||
const courseUpdate = await editUpdate(courseId, data); | ||
dispatch(editCourseUpdate(courseUpdate)); |
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.
On edit, the api call is working as expected, but the data does not include the updates made to update. This is probably connected to the change that you made in the wysiwyg editor.
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.
Fixed
src/course-updates/CourseUpdates.jsx
Outdated
<Layout | ||
lg={[{ span: 9 }, { span: 3 }]} | ||
md={[{ span: 9 }, { span: 3 }]} | ||
sm={[{ span: 9 }, { span: 3 }]} | ||
xs={[{ span: 9 }, { span: 3 }]} | ||
xl={[{ span: 9 }, { span: 3 }]} | ||
> |
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 span should be set to 12 because there is no aside.
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.
Fixed
dispatch(updateSavingStatuses({ createCourseUpdateQuery: RequestStatus.PENDING })); | ||
dispatch(showProcessingNotification(NOTIFICATION_MESSAGES.saving)); | ||
const courseUpdate = await createUpdate(courseId, data); | ||
dispatch(createCourseUpdate(courseUpdate)); |
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.
On edit, the api call is working as expected, but the data does not include the updates made to update. This is probably connected to the change that you made in the wysiwyg editor.
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.
Fixed
<Button variant="tertiary" type="button" onClick={close}> | ||
{intl.formatMessage(messages.cancelButton)} | ||
</Button> | ||
<Button onClick={handleSubmit} type="submit"> |
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.
Button should be disabled if the form cannot be submitted
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.
Fixed
bf98a00
to
ccfd55b
Compare
@vladislavkeblysh 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |