-
Notifications
You must be signed in to change notification settings - Fork 295
feat: hide sidebar on screen resize #1720
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: hide sidebar on screen resize #1720
Conversation
|
Thanks for the pull request, @holaontiveros! 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. Where 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. |
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.
Pull Request Overview
Adds a window resize listener to automatically collapse the course outline sidebar when the viewport is narrower than the sidebar breakpoint.
- Imports
useLayoutEffect,debounce, andbreakpointsto set up resize-based collapsing - Defines a resize handler within a layout effect to trigger
handleToggleCollapsewhen needed - Cleans up the listener on unmount
Comments suppressed due to low confidence (3)
src/courseware/course/sidebar/sidebars/course-outline/hooks.jsx:112
- This resize-based collapse behavior isn't covered by tests; add unit or integration tests to verify the sidebar hides correctly on window resize.
useLayoutEffect(() => {
src/courseware/course/sidebar/sidebars/course-outline/hooks.jsx:5
- Remove the unused
debounceimport or apply it in the resize handler, and clean up the unusedDEBOUNCE_WAITconstant.
import { debounce } from 'lodash';
src/courseware/course/sidebar/sidebars/course-outline/hooks.jsx:23
- The
DEBOUNCE_WAITconstant is defined but not used; remove it or integrate it withlodash.debounceto regulate the resize handler.
const DEBOUNCE_WAIT = 1; // ms
src/courseware/course/sidebar/sidebars/course-outline/hooks.jsx
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1720 +/- ##
==========================================
+ Coverage 90.43% 90.45% +0.01%
==========================================
Files 344 344
Lines 5793 5802 +9
Branches 1394 1398 +4
==========================================
+ Hits 5239 5248 +9
Misses 535 535
Partials 19 19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
arbrandes
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.
I had just noticed this bug when testing another commit - thank you for tackling the problem!
The code looks good in principle, but it doesn't always work. This is one case where it worked:
Success.mp4
Immediately followed by one where it didn't:
Failure.mp4
It seems the actual width the window ends up in may have something to do with it.
This is on Firefox 139.0.1 on Linux, by the way. I wouldn't be completely surprised if this behavior is not reproducible in other browsers/platforms, but I am interested if this is the case.
It's related with the speed the events are triggered I even didn't debounce it to preven that exact failure in chrome, let me try to fine tune it a bit more so it works also in FF |
|
@arbrandes I optimized the code to make sure that it's not toggled the "isOpen" wasn't doing the same there as in Chrome... so I just made sure that the sidebar is properly collapsed and not toggled, the code is still triggered once when the the conditions are met and there's no more flickering. Screen.Recording.2025-06-04.at.9.36.45.a.m.mov |
|
@holaontiveros, thanks for the update! Do you mind rebasing on latest master and resolving conflicts? |
54c4820 to
b292e6c
Compare
|
@arbrandes done! |
arbrandes
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.
This works beautifully! Thank you!
|
Thank you so much for working on this, @holaontiveros! Can you help us backporting this fix to the release/teak branch? We'd really appreciate it! |
fixes #1400
Description
Adds an event handler on the window resize to check if the sidebar isOpen and the size of the viewport is smaller than the sidebar display to hide the sidebar and prevent it from blocking the course view.
This doesn't have any debounce given that the execution only happens when the sidebar is open and the size it's met so the code get's only executed once.
Video
Before the change:
Screen.Recording.2025-05-29.at.12.58.09.p.m.mov
After the change:
Screen.Recording.2025-05-29.at.12.57.27.p.m.mov