feat(AU-2375): Update breadcrumbs and outline navigation bar UI #1582
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1582 +/- ##
==========================================
+ Coverage 84.61% 84.62% +0.01%
==========================================
Files 332 332
Lines 5661 5666 +5
Branches 1391 1394 +3
==========================================
+ Hits 4790 4795 +5
Misses 852 852
Partials 19 19 ☔ View full report in Codecov by Sentry. |
|
@openedx/committers-frontend-app-learning this PR is ready for review |
|
I'll try to take a look by tomorrow. |
There was a problem hiding this comment.
Mostly looks good. Please just swap out courseId in favor of useContextId, and can you please update the slot README to list the new props?
| @@ -161,7 +162,13 @@ const Sequence = ({ | |||
| const defaultContent = ( | |||
| <> | |||
| <div className="sequence-container d-inline-flex flex-row w-100"> | |||
| <CourseOutlineSidebarTriggerSlot /> | |||
| <CourseOutlineSidebarTriggerSlot | |||
| courseId={courseId} | |||
There was a problem hiding this comment.
Per a big discussion on #1496 , I think courseId should not be a prop, but instead any plugins that need it can get it with the useContextId hook. See the example code for ProgressTabCourseGradeSlot which shows this.
Ideally, isStaff would be likewise always available via a useIsStaff or useUser hook, but I don't think it's been implemented/standardized yet.
I think the other props are reasonable for this.
There was a problem hiding this comment.
Regarding your suggestion on using useContextId to get the courseId instead of passing it as a prop to my plugin, I have an issue when trying to use it from the view that's inserting that plugin. I used the hook as in here https://github.com/openedx/frontend-app-learning/pull/1496/files#diff-295513048177ffa3f1a1d414820290303766b7c74b29050d29aea1a08a59305cR19, looks like that hook is using the courseId saved in courseHome from redux store but that data seems to be available from the course home page. When using the Sequence/Unit view, the data is available in courseware and courseHome has null/undefined data. Is there a courseware hook that I could use or should I go back to use the courseId as prop ?
There was a problem hiding this comment.
Could you just update the useContextId hook, so that it falls back? Like this:
- export const useContextId = () => useSelector<RootState>(state => state.courseHome.courseId);
+ export const useContextId = () => useSelector<RootState>(state => state.courseware.courseId ?? state.courseHome.courseId);
|
@bradenmacdonald can you merge this if you have no update requests? The Aurora team does not have merge rights. Thanks! |
|
@jristau1984 I now have merge rights so I can merge it |
Description
Extend CourseOutlineSidebarTriggerSlot props to amplify plugin usage.