Skip to content
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

Fix/lm sticky sidebar #6304

Open
wants to merge 10 commits into
base: trunk
Choose a base branch
from
1 change: 1 addition & 0 deletions assets/course-theme/learning-mode.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import './scroll-direction';
import './adminbar-layout';
import './featured-video-size';
import './sidebar';
import { toggleFocusMode } from './focus-mode';
import { submitContactTeacher } from './contact-teacher';
import { initCompleteLessonTransition } from './complete-lesson-button';
Expand Down
249 changes: 249 additions & 0 deletions assets/course-theme/sidebar.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,249 @@
/**
* External dependencies
*/
import debounce from 'lodash/debounce';

/**
* The last scroll top value.
*
* @member {number}
*/
let lastScrollTop = 0;

/**
* Calculates the scroll delta.
*/
const getScrollDelta = () => {
const { scrollTop } = document.documentElement;
const delta = scrollTop - lastScrollTop;
lastScrollTop = Math.max( 0, scrollTop );
return delta;
};

/**
* Tells if the sidebar is supposed to be sticky.
*
* @return {boolean} True if it is sticky. False otherwise.
*/
const isStickySidebar = () =>
[ 'modern', 'video-full' ].some( ( templateName ) =>
document.body.classList.contains( `learning-mode--${ templateName }` )
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

b5c8462


/**
* Sidebar margin top.
*
* @member {number}
*/
let sidebarMarginTop = 0;

/**
* The sidebar DOM element.
*
* @member {HTMLElement}
*/
let sidebar = null;

/**
* The header DOM element.
*
* @member {HTMLElement}
*/
let header = null;

/**
* The clone of the sidebar DOM element. This is the sidebar element
* that user sees and interacts with.
*
* @member {HTMLElement}
*/
let stickySidebar = null;

/**
* The featured video DOM element.
*
* @member {HTMLElement}
*/
let featuredVideo = null;

/**
* Populates the DOM elements that we need.
*/
const queryDomElements = () => {
sidebar = document.querySelector( '.sensei-course-theme__sidebar' );
header = document.querySelector( '.sensei-course-theme__header' );
featuredVideo = document.querySelector(
'.sensei-course-theme-lesson-video'
);
};

/**
* 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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😅

4cbd7b6

* is used to keep the sidebar sticky. We still need the original sidebar
* element because we use it's original position to calculate and decide
* where the stickySideber should be position at any given time.
*
* This can be called multiple times and if it detects an existing stickySidebar
* present in the DOM it will remove it and insert the new one.
*/
function preparestickySidebar() {
const sidebarRect = sidebar.getBoundingClientRect();
sidebarMarginTop = sidebar.style.marginTop
? parseInt( sidebar.style.marginTop, 10 )
: 0;
if ( stickySidebar?.remove ) {
stickySidebar.remove();
}
stickySidebar = sidebar.cloneNode( true );
stickySidebar.style.position = 'fixed';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah. I tried the position: sticky approach initially. I couldn't make it work properly.

stickySidebar.style.opacity = 1;
stickySidebar.style.zIndex = 2;
stickySidebar.style.top = `${ sidebarRect.top }px`;
stickySidebar.style.left = `${ sidebarRect.left }px`;
stickySidebar.style.width = `${ sidebarRect.right - sidebarRect.left }px`;
stickySidebar.style.transition = 'none';
sidebar.parentElement.append( stickySidebar );
sidebar.style.opacity = 0;
}

/**
* Sidebar bottom margin.
*
* @member {number}
*/
const SIDEBAR_BOTTOM_MARGIN = 32;

/**
* Updates the stickySidebar position. The position of the stickySidebar
* is relative to the Learning Mode header block. It assumes the header is
* fixed.
*
* @param {boolean} initialPosition True if the sidebar should be positioned
* for it's initial position given the current
* state of the scrollbar. Used when user opens
* the page and the page is scrolled into the middle.
*/
function updateSidebarPosition( initialPosition = false ) {
if ( ! sidebar || ! stickySidebar ) {
return;
}

// Get the current dimensions of the elements.
const headerRect = header.getBoundingClientRect();
const sidebarRect = sidebar.getBoundingClientRect();
const stickySidebarRect = stickySidebar.getBoundingClientRect();

// Calculate required values.
const delta = getScrollDelta();
const stickySidebarHeight =
stickySidebarRect.bottom - stickySidebarRect.top;
const stickySidebarIsTallerThanViewport =
stickySidebarHeight >
window.innerHeight -
( headerRect.bottom + sidebarMarginTop + SIDEBAR_BOTTOM_MARGIN );
let stickySidebarNewTop = sidebarRect.top;

// If the sidebar is very tall and does not fit into the viewport vertically
// we scroll the sticky sidebar up until the bottom is reached. Or we scroll
// the sticky sidebar down until the top of the sidebar is reached.
if ( stickySidebarIsTallerThanViewport && ! initialPosition ) {
stickySidebarNewTop = stickySidebarRect.top - delta;
const stickySidebarNewBottom = stickySidebarRect.bottom - delta;
const stickySidebarMinTop = sidebarRect.top;
const stickySidebarMinBottom =
window.innerHeight - SIDEBAR_BOTTOM_MARGIN;

// The sidebar is moving upwards.
if ( delta >= 0 ) {
if ( stickySidebarNewBottom < stickySidebarMinBottom ) {
stickySidebarNewTop =
stickySidebarMinBottom - stickySidebarHeight;
}

// The sidebar is moving downwards.
} else {
if ( stickySidebarNewTop > headerRect.bottom ) {
stickySidebarNewTop = headerRect.bottom;
}
if ( stickySidebarNewTop < stickySidebarMinTop ) {
stickySidebarNewTop = stickySidebarMinTop;
}
}

// If the sidebar fits into the viewport vertically
// then we simply stick it below the header when user
// scrolls it up above the header.
} else if ( sidebarRect.top <= headerRect.bottom ) {
stickySidebarNewTop = headerRect.bottom;

// By default we position the sticky sidebar on top
// of the original sidebar.
} else {
stickySidebarNewTop = sidebarRect.top;
}

// Need to subtract the sidebar top margin because fixed positioned elements
// are pushed down by css top margin.
stickySidebar.style.top = `${ stickySidebarNewTop - sidebarMarginTop }px`;
}

/**
* Reinitializes the sticky sideber
*/
const reinitializeSidebar = debounce( () => {
preparestickySidebar();
updateSidebarPosition( true );
}, 500 );

/**
* Makes sure the height of the sidebar is at least the height
* of the featured video in 'modern' LM template.
*/
function syncSidebarSizeWithVideo() {
if ( featuredVideo && sidebar ) {
new window.ResizeObserver( () => {
const videoHeight = featuredVideo.offsetHeight;
const sidebarHeight = sidebar.offsetHeight;
if (
! videoHeight ||
! sidebarHeight ||
sidebarHeight >= videoHeight
) {
return;
}
sidebar.style.height = `${ videoHeight }px`;
reinitializeSidebar();
} ).observe( featuredVideo );
}
}

/**
* Makes the sidebar sticky for relevant LM templates.
*/
function setupStickySidebar() {
if ( ! isStickySidebar() ) {
return;
}

queryDomElements();

document.defaultView.addEventListener( 'scroll', () =>
updateSidebarPosition()
);

// eslint-disable-next-line @wordpress/no-global-event-listener
window.addEventListener( 'resize', reinitializeSidebar );

// Make sure sidebar height is not shorter than the video height
// for `moderm` lm template.
if ( document.body.classList.contains( 'learning-mode--modern' ) ) {
syncSidebarSizeWithVideo();
}
yscik marked this conversation as resolved.
Show resolved Hide resolved

reinitializeSidebar();
}

// eslint-disable-next-line @wordpress/no-global-event-listener
window.addEventListener( 'DOMContentLoaded', setupStickySidebar );
13 changes: 12 additions & 1 deletion includes/course-theme/class-sensei-course-theme-templates.php
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public function init() {
add_filter( 'pre_get_block_file_template', [ $this, 'get_single_block_template' ], 10, 3 );
add_filter( 'theme_lesson_templates', [ $this, 'add_learning_mode_template' ], 10, 4 );
add_filter( 'theme_quiz_templates', [ $this, 'add_learning_mode_template' ], 10, 4 );

add_filter( 'body_class', [ $this, 'add_body_class' ], 10, 2 );
}


Expand Down Expand Up @@ -499,5 +499,16 @@ private function should_hide_lesson_template( $post_type ) {
return false;
}

/**
* Adds the active template class to body tag.
*
* @param array $classes The list of body class names.
* @param array $class The list of additional class names added to the body.
*/
public function add_body_class( array $classes, array $class ): array {
$active_template_name = Sensei_Course_Theme_Template_Selection::get_active_template_name();
$classes[] = "learning-mode--{$active_template_name}";
return $classes;
}

}