Fix/lm sticky sidebar #6304
Fix/lm sticky sidebar #6304
Conversation
Codecov Report
@@ Coverage Diff @@
## trunk #6304 +/- ##
============================================
- Coverage 45.97% 45.97% -0.01%
- Complexity 9551 9552 +1
============================================
Files 459 459
Lines 33837 33842 +5
Branches 275 275
============================================
Hits 15558 15558
- Misses 18074 18079 +5
Partials 205 205
Continue to review full report at Codecov.
|
assets/course-theme/sidebar.js
Outdated
| * Creates an exact copy of the sidebar DOM element | ||
| * and sets it's position to fixed. The original sidebar | ||
| * element is hidden by seting it's opacity to 0. The clone, "stickySidebar" |
There was a problem hiding this comment.
Could we have add some placeholder element instead, to avoid having the sidebar duplicated? If not, let's try to make this accessible and ensure the hidden element is not causing issues. visibility: hidden instead of the opacity change will still keep the element it in the layout, but remove it from the accessibility tree. I think you also can't TAB into it, but not 100% sure. (And could also add aria-hidden to make it explicit.)
There was a problem hiding this comment.
Good idea. I went with the placeholder element approach. Much better now. It also fixes the bug where you couldn't expand/collapse course modules in the sidebar. I realized it is not working just today. 😅
assets/course-theme/sidebar.js
Outdated
| stickySidebar.remove(); | ||
| } | ||
| stickySidebar = sidebar.cloneNode( true ); | ||
| stickySidebar.style.position = 'fixed'; |
There was a problem hiding this comment.
Would position: sticky be usable as a starting point? The browser would take away some of the positioning math and adjustments. Or is it harder to work with?
There was a problem hiding this comment.
Nah. I tried the position: sticky approach initially. I couldn't make it work properly.
assets/course-theme/sidebar.js
Outdated
| [ 'modern', 'video-full' ].some( ( templateName ) => | ||
| document.body.classList.contains( `learning-mode--${ templateName }` ) | ||
| ); |
There was a problem hiding this comment.
Could we use something like an .is-sticky class on the sidebar block instead? Coding it to template names makes it hard to figure out where this behavior is coming from and why it's only happening in some cases.
With an .is-sticky class, we can also build it into a setting toggle in the editor later.
(This does mean changing the templates, which won't affect folks who customized it already, but that's probably for the best.)
There was a problem hiding this comment.
You're probably right. It's better to have that info close to the sidebar blocks.
Although the block is not strictly a sidebar block. It's variation of the sensei-lms/ui block. I'm sure there is a way to add some sort of block setting depending on the block variation. I didn't spend time to figure that out though. Didn't want to turn this into bigger project.
What I did instead is I added a custom class name in the Advanced section of the Sidebar Menu block. So it's on by default and can be removed by the user if they want to. Maybe we can turn it into a block setting with a better UI. For now I think this approach is sufficient.
Checkout the change in the sensei pro repo here: https://github.com/Automattic/sensei-pro/pull/1988
|
Hey @yscik do you think this needs more work to get it merged? |
WordPress Dependencies ReportThe
This comment was automatically generated by the |
|
Code looks good! Did some testing, logging a few issues here that we should look into: When the content is shorter than the sidebar, the sidebar bottom is cut off and can't be accessed:Sticky-sidebar-short-content.movWeird issue of a contact teacher thing appearing:Sticky-contact-teacher.mov |
Fixes #5912
Changes proposed in this Pull Request
modernandfull-videotemplates sticky.moderntemplate if featured video is taller than the sidebar, then it makes the sidebar the same height.Testing instructions
Screenshot / Video
Regular Sticky Sidebar
sticky-sidebar-regular.mp4
Full Video Sticky Sidebar
sticky-sidebar-full-video.mp4
Height Overflow Sticky Sidebar
sticky-sidebar-height-overflow.mp4
Video Height Sticky Sidebar
sticky-sidebar-video-height.mp4
Video Full Height Overflow Sticky Sidebar
sticky-sidebar-full-video-height-overflow.mp4