feat: Course outline restructure with new plugin slots - #1920
Conversation
openedx-webhooks
added
open-source-contribution
|
Thanks for the pull request, @xitij2000! 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. |
44ec042 to
cd853d0
Compare
xitij2000
changed the title
xitij2000
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1920 +/- ##
==========================================
+ Coverage 92.86% 92.88% +0.01%
==========================================
Files 360 360
Lines 5890 5901 +11
Branches 1407 1410 +3
==========================================
+ Hits 5470 5481 +11
Misses 402 402
Partials 18 18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
itsjeyd
added
the
waiting for eng review
|
@brian-smith-tcril Did you get a chance to look at this PR? |
Not quite yet. I've been focused on a release testing bug fix. I definitely haven't forgotten about it and it's still high on my todo list. |
brian-smith-tcril
left a comment
brian-smith-tcril
left a comment
There was a problem hiding this comment.
Overall these extension points seem like good ones to add. Looking forward to continuing to discuss these changes and finding a good path for them to land!
|
|
||
| return ( | ||
| <li className={classNames({ 'bg-info-100': isActive, 'border-top border-light': !isFirst })}> | ||
| <li className={classNames({ 'active-unit bg-info-100': isActive, 'border-top border-light': !isFirst })}> |
There was a problem hiding this comment.
active-unit doesn't appear to exist.
|
Just giving context on some of the issues:
I tried splitting this PR into two parts, however there were quite a few dependencies between the commits so it wasn't as easy a split as I hoped. I'll apply the other fixes first. |
dc917eb to
d627014
Compare
I'm open to discussing those changes, but I'd like to keep this PR focused on adding the plugin slots. By keeping the PR focused we won't have changes blocking each other from landing.
That's probably fine, but I'd still prefer to have this as a "just adding slots" PR.
I'm sorry about that. I know it can be frustrating to split things up, but I think the benefits of focused PRs are very real. |
|
@brian-smith-tcril I've split out the slots into this PR: #1933 A test is failing locally so I still have work to do, but I'll rebase this PR once that one is merged. |
itsjeyd
added
blocked by other work
c3daaa0 to
f59ca84
Compare
| variant="tertiary" | ||
| className={classNames( | ||
| 'd-flex align-items-center w-100 px-4 py-3.5 rounded-0 justify-content-start', | ||
| { 'bg-info-100': isActiveSection }, |
There was a problem hiding this comment.
This is now applied via SCSS
cc980c5 to
614ced5
Compare
|
@brian-smith-tcril I've updated this PR. FYI: the update to the |
itsjeyd
added
waiting for eng review
|
Hi @brian-smith-tcril, would you have time to take another look at this PR? |
|
@itsjeyd I took another look and resolved a lot of my old comments. @xitij2000 this PR has a conflict in |
Splits useCourseOutlineSidebar into useCourseOutlineData and useCourseOutlineSidebar for improved separation of responsibilities. Updates components to reflect this change. This allows the course outline to be used outside of a sidebar context.
614ced5 to
7053d03
Compare
|
@brian-smith-tcril That conflict had to do with the styling changes which I've removed. This is ready now. I'll prepare another PR for the styling changes. |
brian-smith-tcril
left a comment
brian-smith-tcril
left a comment
There was a problem hiding this comment.
I finally feel like I was able to wrap my head around these changes. Overall it's looking pretty good. I left a few comments in there with suggestions/questions, looking forward to hearing your thoughts!
| interface CoursePageParams extends Record<string, string> { | ||
| courseId: string; | ||
| unitId: string; | ||
| } |
There was a problem hiding this comment.
Claude called this out
The non-optional declarations do nothing.
courseId: string; unitId: stringcome back throughPartial<>asstring | undefinedregardless. That's precisely why the author still had to writecourseId!andunitId!at lines 100 and 103 — the interface claims the params are always present, the type system disagrees, and the!papers over the gap. The interface is stating a guarantee it can't deliver, and the route/course/:courseId/:sequenceId(constants.ts:16) meansunitIdgenuinely is absent sometimes.
| const handleClick = React.useCallback(() => { | ||
| // Hide the sidebar after selecting a unit on a mobile device. | ||
| if (shouldDisplayFullScreen) { | ||
| handleToggleCollapse(); | ||
| } | ||
| handleUnitClick({ sequenceId, activeUnitId, id }); | ||
| }, [handleUnitClick, sequenceId, activeUnitId, id, shouldDisplayFullScreen, handleToggleCollapse]); |
There was a problem hiding this comment.
Looking at this block a few things stand out to me:
- It's no longer inline
- This makes sense, we used to have the
handleToggleCollapseinhandleUnitClick, now that needs to be called here instead, making it a named function seems reasonable.
- This makes sense, we used to have the
- The
if (shouldDisplayFullScreen) { handleToggleCollapseblock is beforehandleUnitClick- That block was at the end of
handleUnitClickbefore. I don't think there's a behavioral difference with it moving, but it stood out to me as a difference to investigate.
- That block was at the end of
- It is now using
useCallbackinstead of a plain arrow function.- It's not clear to me what this is buying us. I'm open to the change if there's clear justification, but I'd think sticking to a plain arrow function here would be fine.
Claude dive into why useCallback isn't buying us anything
- Something re-renders
UnitLinkWrapper.UnitLinkWrapper()runs again, souseCourseOutlineData()runs again.- That hook's line
const handleUnitClick = (...) => {...}executes again → allocates a different function object than last render.useCallbackcompares this render's deps to last render's withObject.is.handleUnitClickis a different object, so that check fails.- Cache miss →
useCallbackreturns a newly createdhandleClick.
| const [selectedSection, setSelectedSection] = useState<string | null>(null); | ||
| const [isDisplaySequenceLevel, setDisplaySequenceLevel, setDisplaySectionLevel] = useToggle(true); | ||
|
|
||
| const { unitId, courseId } = useParams<CoursePageParams>(); |
There was a problem hiding this comment.
I don't see any test changes in this PR. Were there tests assuming we were getting a unitId from useCourseOutlineSidebar before that need to be updated to have one provided by params?
There was a problem hiding this comment.
Yes, that is a good point. I think the fact that not tests fail after moving the source from Sidebar context to useParams means that the tests were probably not testing how unitId is used.
| return null; | ||
| } | ||
| return <CourseOutline />; | ||
| return <CourseOutline shouldDisplayFullScreen={shouldDisplayFullScreen} onToggleCollapse={handleToggleCollapse} />; |
There was a problem hiding this comment.
Claude question about this:
Why props here instead of having CourseOutline call useCourseOutlineSidebar() directly? Every other consumer in this subtree — the trigger, UnitLinkWrapper — reads the sidebar hook itself, so CourseOutline becomes the only component being fed these two values from outside. If the intent is to make CourseOutline renderable outside a sidebar, could the props fall back to the context values when they aren't passed, rather than to false/undefined? That way the sidebar case keeps working through the hook as it does today (and CourseOutlineTray doesn't need to pass anything at all), and the props become an explicit override for the out-of-sidebar case.
There was a problem hiding this comment.
Having CourseOuline call useCourseOutlineSidebar is the exact reason for this refactoring in the first place, we want to make it independent of any sidebar logic. Having it still get it from the SidebarContext would partially defeat the purpose of the split.
There was a problem hiding this comment.
Having CourseOuline call
useCourseOutlineSidebaris the exact reason for this refactoring in the first place, we want to make it independent of any sidebar logic. Having it still get it from the SidebarContext would partially defeat the purpose of the split.
Could you elaborate on this a bit? My feeling is that regardless of where CourseOutline gets shouldDisplayFullScreen and handleToggleCollapse from, by caring about those at all it's including "sidebar logic."
I'm open to these being passed as props, I just want to understand the motivation a bit better.
There was a problem hiding this comment.
I see your point, that is how those values are currently used. However for a client we extracted this component and injected it in the header as a dropdown. It can still use the toggle collapse, but its context is different from that of a sidebar. This work was triggered by the need for hosting this component outside the sidebar. Having the values passed explicitly seemed to be a cleaner separation.
That said it's still in the sidebars folder so without further refactoring it's still somewhat tied to the sidebar and keeping the values by default won't break anything.
We could do further refactoring to move this out and make the fullscreen code part of the sidebar wrapper rather than the outline component, but I think for now I can make these params default to the sidebar context values.
There was a problem hiding this comment.
I haven't read through this file fully but it looks like it might need to be updated, so this is just a note to make sure this file accurately reflects the current state of the repo with the changes in this PR.
4 participants
Splits
useCourseOutlineSidebarintouseCourseOutlineData+useCourseOutlineSidebar, addsCourseOutlineSidebarHeadingSlot,CourseOutlineSidebarSectionCompletionIconSlot, andCourseOutlineSidebarSequenceCompletionIconSlot, and introduces CSS variables for theming the sidebar.