fix: navigation sidebar scrolling on mobile devices - #1927
Conversation
|
Thanks for the pull request, @Agrendalath! 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1927 +/- ##
=======================================
Coverage 91.48% 91.49%
=======================================
Files 353 353
Lines 5838 5843 +5
Branches 1356 1357 +1
=======================================
+ Hits 5341 5346 +5
Misses 478 478
Partials 19 19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hey @Agrendalath, this looks like a clear-scope bug fix. Could you check the instructions for that contribution path and complete the remaining steps (right now that would just be #1 and #3). |
|
@itsjeyd, can I move it to the engineering review myself, or should I ping you to double-check first? |
76c539e to
ebb5378
Compare
|
@Agrendalath You're welcome to update the status yourself :) However, having cleared product review this would go to Ready for Review now, and only move to In Eng Review once an upstream reviewer has been confirmed and they've started reviewing. |
|
@openedx/committers-frontend-app-learning This is ready for you. |
brian-smith-tcril
left a comment
There was a problem hiding this comment.
Thanks for the PR!
It looks like this has some conflicts now (likely because #1923 landed). If you could rebase this that would be perfect!
I also left a comment with a small question.
farhaanbukhsh
left a comment
There was a problem hiding this comment.
The code changes look good to me and I have tested them as well. Can we add some tests here @Agrendalath ?
ebb5378 to
1b7965b
Compare
1b7965b to
56ba046
Compare
|
@brian-smith-tcril, @farhaanbukhsh, I replaced the SCSS override with the Paragon class and added tests. |
farhaanbukhsh
left a comment
There was a problem hiding this comment.
👍
✅ I tested this: on the master devstack
- The sidebar closing issue is resolved
- The sidebar takes the whole screen
✅ I read through the code
✅ I checked for accessibility issues
❌ Includes documentation
|
@brian-smith-tcril do you want to review this PR again? |
| }, [courseId, courseOutlineShouldUpdate]); | ||
|
|
||
| // Collapse sidebar if screen resized to a width that displays the sidebar automatically | ||
| const lastWindowWidth = useRef(global.innerWidth); |
There was a problem hiding this comment.
@brian-smith-tcril, I like this idea. However, I'm not sure about all the consequences. E.g., it would likely change existing behavior (e.g., when rotating the phone) beyond what we can consider a bug fix. A follow-up refactor would be more suitable for such a change.
|
One other thing I haven't tested but want to look into is if this changes the behavior on mobile when rotating the device (both switching from landscape to portrait and from portrait to landscape). |
|
@brian-smith-tcril @Agrendalath I tried changing the orientation on the simulator and it is working as expected. Screen.Recording.2026-08-18.at.5.28.41.PM.mov |
The expectation is that the sidebar closes when rotating the device? Also that video appears to show a full refresh on rotate which I wouldn't expect on mobile hardware. |
|
@brian-smith-tcril, this PR does not modify the existing behavior - i.e., the sidebar closes when rotating the device (even though it's indeed confusing). This only fixes the listed issues that are clear bugs that do not require an extended product review.
It appears to be a simulator behavior; I cannot reproduce it on a physical device. |
Fixes #1935.
This fixes the following issues with the navigation sidebar on mobile devices:
resizeevent when hiding the URL bar.fixed-toputility class was overridden by theposition: relativerule for the.outline-sidebar-wrapperselector.Testing instructions
Check the use cases described above with the narrow viewport.
Deadline
"None"
Other information
Private-ref: BB-7413