-
Notifications
You must be signed in to change notification settings - Fork 179
feat: course outline sidebar #2731
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: course outline sidebar #2731
Conversation
|
Thanks for the pull request, @rpenido! 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. |
3f73530 to
3813afd
Compare
8e36e8a to
b9d3ab5
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2731 +/- ##
==========================================
+ Coverage 94.92% 94.96% +0.03%
==========================================
Files 1239 1251 +12
Lines 28452 28691 +239
Branches 6481 6542 +61
==========================================
+ Hits 27009 27246 +237
- Misses 1372 1374 +2
Partials 71 71 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d33b151 to
9ebe024
Compare
71fe4af to
7a844c0
Compare
7a844c0 to
9fe1b28
Compare
Co-authored-by: Navin Karkera <navin@opencraft.com>
5d9da91 to
874e90e
Compare
874e90e to
98c2e51
Compare
000c4f1 to
0e5468a
Compare
0e5468a to
32fd819
Compare
ChrisChV
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.
Looks good, by this #2622 (comment) some changes will likely be needed. I'll keep the pull requests open until I have those designs.
| courseId: string; | ||
| } | ||
|
|
||
| const OutlineHelpSideBar = ({ courseId }: OutlineHelpSidebarProps) => { |
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.
@rpenido you can use:
const { courseId } = useCourseAuthoringContext();
instead of passing it into the 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.
|
|
||
| import messages from './messages'; | ||
|
|
||
| export const OutlineInfoSidebar = ({ courseId }: { courseId: string }) => { |
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.
Same, you can use:
const { courseId } = useCourseAuthoringContext();
instead of passing it into the 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.
Fixed here: f970c84
| courseId: string; | ||
| } | ||
|
|
||
| const OutlineSideBar = ({ courseId }: OutlineSideBarProps) => { |
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.
Same, you can use:
const { courseId } = useCourseAuthoringContext();
instead of passing it into the 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.
Fixed here: f970c84
| sidebarButtonHelp: { | ||
| id: 'course-authoring.course-outline.sidebar.sidebar-button-help', | ||
| defaultMessage: 'Help', | ||
| }, | ||
| sidebarButtonInfo: { | ||
| id: 'course-authoring.course-outline.sidebar.sidebar-button-info', | ||
| defaultMessage: 'Info', | ||
| }, |
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.
It is necessary to add the descriptions here
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 here: f970c84
| import { Stack } from '@openedx/paragon'; | ||
|
|
||
| interface SidebarContentProps { | ||
| children: React.ReactNode[]; |
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.
@rpenido This needs to be updated to accept content from a single SidebarSection
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 here: f970c84
e24ee8c to
f970c84
Compare
f970c84 to
c607da0
Compare
ChrisChV
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.
The code looks good! 👍
Description
This PR implements the new sidebar design for the Course Outline.
"Course Author" and "Developer"
Supporting information
Testing instructions
ENABLE_COURSE_OUTLINE_NEW_DESIGNflag is enabledENABLE_COURSE_OUTLINE_NEW_DESIGNflag toFalseand check if the old Help Sidebar is shownBest Practices Checklist
We're trying to move away from some deprecated patterns in this codebase. Please
check if your PR meets these recommendations before asking for a review:
.ts,.tsx).propTypesanddefaultPropsin any new or modified code.src/testUtils.tsx(specificallyinitializeMocks)apiHooks.tsin this repo for examples.messages.tsfiles have adescriptionfor translators to use.../in import paths. To import from parent folders, use@src, e.g.import { initializeMocks } from '@src/testUtils';instead offrom '../../../../testUtils'Private ref: FAL-4291