Make sidebar container scroll internally #554

adamwoodnz · 2024-01-10T02:55:14Z

This PR modifies the sidebar container so that once it becomes fixed, it also scrolls internally. When the footer collides with the bottom of the sidebar, the sidebar reduces in height rather than starting to scroll with the page.

It has been decoupled from the Table of Contents so that it can be used for the Developer Chapter List as well. JS changes are required to enable handling multiple sidebar containers on the page.

The scrolling areas have custom scrollbars so that they appear on hover. This has been inspired by the React docs (example), and should help users discover that these are scrollable more easily. With the typical Mac OS setting of hidden scrollbars, it is not immediately obvious. Note that this does not work in Firefox.

Overall this is a much simpler implementation, with no top positioning required, and no listener for main height changes.

Testing

The Sidebar Container is currently used on Developer Resources and Documentation.

Things to test

Fixing and scrolling on pages with long and short ToCs and Chapter Lists
Expand and collapse behaviour of ToC on small screens
Scrollbars being shown on hover
Show more/less Related items on Code Reference pages like http://localhost:8888/reference/functions/is_admin/

Use WordPress/wporg-developer#447 to test Developer Resources

Documentation requires no changes, so you can use that local environment as is, and switch the mu-plugins install to this branch.

mu-plugins/blocks/sidebar-container/src/view.js

renintw · 2024-01-22T06:50:16Z

I'm not sure if this has been discussed, but currently, the scrollbar appears only when the sidebar is fixed as described. What about the situation shown in the image below? At this point, the sidebar isn't fixed yet, but the chapter list is already overflowing.

renintw

Tested all the cases suggested in the description. Works as expected 👍
Left some comments, but none of them are blockers.

renintw · 2024-01-22T07:47:09Z

mu-plugins/blocks/sidebar-container/postcss/style.pcss

-			margin-top: 0;
+			.is-link-to-top {
+				display: block;
+				padding-top: var(--wp--preset--spacing--20);


We can also consider removing the extra padding when there's no TOC content.

mu-plugins/blocks/sidebar-container/postcss/style.pcss

ryelle · 2024-01-23T22:20:22Z

mu-plugins/blocks/sidebar-container/postcss/style.pcss

 		margin-bottom: 0 !important;
+		padding: var(--wp--preset--spacing--20) 0;
+
+		main & {


This seems to be the cause of this issue for me— this isn't sass, so the nesting rules aren't the same. This seems to be ignored by postcss, and then stripped out by cssnano, so the rules below are applied to all .wp-block-wporg-sidebar-containers. I don't know why it works for you & @renintw though.

If I move this code to a new section main .wp-block-wporg-sidebar-container {… it works as expected.

I don't know why it works for you & @renintw though.

Nice catch! All this seems to work with a dev build, but it all breaks with a prod build. Starting to think moving to one CSS style would be a really good idea as switching feature sets contributes to this sort of issue. I think that was suggested to be postcss.

I think that was suggested to be postcss.

I think that was before wp-scripts natively supported sass… and maybe if we switched to sass here, we could remove the custom build script here.

mu-plugins/blocks/sidebar-container/postcss/style.pcss

adamwoodnz · 2024-01-24T23:39:57Z

I'm not sure if this has been discussed, but currently, the scrollbar appears only when the sidebar is fixed as described. What about the situation shown in the image below? At this point, the sidebar isn't fixed yet, but the chapter list is already overflowing.

My expectation is that the user would do the default and start scrolling the page, as they wouldn't expect the Chapter List to scroll until that behaviour is observed.

Block.editor.chapter.scroll.mp4

Make sense?

renintw · 2024-01-25T02:11:37Z

My expectation is that the user would do the default and start scrolling the page, as they wouldn't expect the Chapter List to scroll until that behaviour is observed.

I see your idea. Thanks for the explanation with the screencast👍
Was originally thought that users who are already familiar with this page might expect to be able to scroll directly through the chapter lists, similar to the React Docs. But the current setup also makes sense, as more list items can actually be viewed by scrolling as well.

Co-authored-by: Kelly Dwan <[email protected]>

This allows individual containers to become inline at different breakpoints, eg. the Chapter List remaining fixed and scrolling when the ToC is already displayed inline.

adamwoodnz · 2024-01-30T04:09:34Z

In order for the Chapter List container to be fixed and scrolling while the ToC container is inline, the fixed styles have been decoupled from a specific breakpoint and managed via a class instead. The breakpoint for each container to switch between inline and fixed/floating can now be configured via a block attribute.

In making this solution work on iPads in particular, I found that the change in the heights of the admin bar and global nav, and the sticky behaviour of the local nav bar and admin bar make for very complex calculations. A resize handler has been added to set some these changing variables on load, and update on resize to keep everything in sync. A such you should also be able to test switching orientation, not only page load.

These changes can be viewed in b24b6f2.

adamwoodnz · 2024-01-30T04:12:21Z

mu-plugins/blocks/local-navigation-bar/postcss/style.pcss

+@media (min-width: 890px) {
+	/* stylelint-disable selector-id-pattern */
+	#wp--skip-link--target {
+		scroll-margin-top: var(--wp--custom--local-navigation-bar--spacing--height, 0);
+	}
+}
+


Moved from the sidebar container styles, as it relates directly to the change in positioning of the local nav bar

adamwoodnz · 2024-01-30T04:19:04Z

mu-plugins/blocks/sidebar-container/src/view.js

-	// If there is no table of contents, hide the heading.
-	if ( ! document.querySelector( '.wp-block-wporg-table-of-contents' ) ) {
-		const heading = document.querySelector( '.wp-block-wporg-sidebar-container h2' );
-		heading?.style.setProperty( 'display', 'none' );
-	}


Moved to the render

Completes the states and fixes issues in Firefox

…ixed

ryelle

Two pretty minor notes, but overall this is working well in my testing.

I did notice a bit of glitchy behavior when the content is shorter than the Chapters list, and the screen size is just tall enough to render chapters without scrolling, in this case ~1000-1100ish. The sidebar bounces back and forth between scroll/static.

scroll-glitch.mp4

Marking this as approved, if you'd rather fix in a follow-up PR.

ryelle · 2024-01-31T21:24:04Z

mu-plugins/blocks/sidebar-container/index.php

+			esc_html__( '↑ Back to top', 'wporg' )
+		)
+		: '';
+	$inlineBreakpoint = $attributes['inlineBreakpoint'];


Suggested change

$inlineBreakpoint = $attributes['inlineBreakpoint'];

$inline_breakpoint = $attributes['inlineBreakpoint'];

PHP variables should be snake case. It looks like we're not running phpcs in the github actions, but you can run it locally with composer lint mu-plugins/blocks/sidebar-container mu-plugins/blocks/table-of-contents

Fixed in 2aed927

ryelle · 2024-01-31T21:36:36Z

mu-plugins/blocks/table-of-contents/index.php

+	// If there is no content, don't render the heading.
+	$content .= empty( trim( wp_strip_all_tags( $post_content ) ) )
+		? '' 
+		: do_blocks(
+			'<!-- wp:heading {"style":{"typography":{"fontStyle":"normal","fontWeight":"400"},"spacing":{"margin":{"top":"0","bottom":"0"}}},"fontSize":"normal","fontFamily":"inter"} -->
+			<h2 class="wp-block-heading has-inter-font-family has-normal-font-size" style="margin-top:0;margin-bottom:0;font-style:normal;font-weight:400">' . esc_html( $title ) . '</h2>
+			<!-- /wp:heading -->'
+		);


Shouldn't the if ( ! $items ) check prevent the entire block (incl heading) from being output? I know this is coming from the previous JS, but I'm not sure how we'd end up in a situation where empty( trim( wp_strip_all_tags( $post_content ) ) ) is true but ! $items is false.

Nice catch, I moved the logic without checking what would happen just removing the JS. Works just as you described, reverted the render change in 07b512c

… render" This reverts the render changes in commit 740a0aa, but keeps the JS changes

This adds space between sidebar and footer when main content is short

adamwoodnz added this to the Developer redesign: Iteration 1 milestone Jan 10, 2024

adamwoodnz added the Redesign Related to the wordpress.org redesign project label Jan 10, 2024

adamwoodnz self-assigned this Jan 10, 2024

adamwoodnz force-pushed the enhance/scrolling-sidebar branch 5 times, most recently from b9309f7 to 48fee8f Compare January 12, 2024 02:01

adamwoodnz commented Jan 12, 2024

View reviewed changes

mu-plugins/blocks/sidebar-container/src/view.js Show resolved Hide resolved

adamwoodnz commented Jan 12, 2024

View reviewed changes

mu-plugins/blocks/sidebar-container/src/view.js Outdated Show resolved Hide resolved

adamwoodnz mentioned this pull request Jan 12, 2024

Chapters and ToC: Internal scroll WordPress/wporg-developer#469

Merged

adamwoodnz force-pushed the enhance/scrolling-sidebar branch from 8bc9f39 to f01b836 Compare January 12, 2024 04:57

adamwoodnz marked this pull request as ready for review January 16, 2024 04:19

adamwoodnz requested review from ryelle, renintw and StevenDufresne January 16, 2024 04:20

adamwoodnz added [Block] Table of Contents [Block] Sidebar Container labels Jan 16, 2024

renintw approved these changes Jan 22, 2024

View reviewed changes

ryelle reviewed Jan 23, 2024

View reviewed changes

ryelle mentioned this pull request Jan 29, 2024

Tools: Update packages, use wp-scripts for build process #562

Merged

adamwoodnz force-pushed the enhance/scrolling-sidebar branch 3 times, most recently from 1f0b446 to 75ef37e Compare January 30, 2024 03:21

adamwoodnz added 3 commits January 30, 2024 16:23

Make sidebar container scroll internally

c79eae4

Decouple sidebar container from ToC

1fd41c1

Allow multiple sidebar containers

b05e933

adamwoodnz and others added 8 commits January 30, 2024 16:23

Make back to top link optional

49d1fac

Update docs

0eb7dea

Fix postcss selector nesting

08191e0

Co-authored-by: Kelly Dwan <[email protected]>

Fix postcss selector nesting

c914570

Co-authored-by: Kelly Dwan <[email protected]>

Fix postcss selector nesting

f1a29d9

Remove ToC back to top padding if no content

f291ec1

Make sidebar container control padding when fixed

ebb5561

Move skip link target styles to local nav bar

7c2c1a7

adamwoodnz force-pushed the enhance/scrolling-sidebar branch from 75ef37e to f5c8a52 Compare January 30, 2024 03:24

adamwoodnz added 3 commits January 30, 2024 16:54

Make the inline breakpoint configurable for each container

b24b6f2

This allows individual containers to become inline at different breakpoints, eg. the Chapter List remaining fixed and scrolling when the ToC is already displayed inline.

Move logic to hide ToC heading if content is empty from JS to render

740a0aa

Fix handling of zero css values in getCustomPropValue

0b9b873

adamwoodnz force-pushed the enhance/scrolling-sidebar branch from fbdf21b to 0b9b873 Compare January 30, 2024 03:55

adamwoodnz requested review from ryelle and renintw January 30, 2024 04:09

adamwoodnz commented Jan 30, 2024

View reviewed changes

Complete jsdoc

17d0add

adamwoodnz force-pushed the enhance/scrolling-sidebar branch from 9de411f to 17d0add Compare January 30, 2024 04:17

adamwoodnz commented Jan 30, 2024

View reviewed changes

adamwoodnz added 4 commits January 30, 2024 17:28

Fix scrolling height calc when logged out

abf6ac5

Revert changing admin bar height detection

91fe052

Improve scrollbar styles

061aab3

Completes the states and fixes issues in Firefox

Only reduce the height when the footer is visible if the sidebar is f…

bcb5688

…ixed

ryelle approved these changes Jan 31, 2024

View reviewed changes

adamwoodnz added 3 commits February 1, 2024 11:31

Revert "Move logic to hide ToC heading if content is empty from JS to…

07b512c

… render" This reverts the render changes in commit 740a0aa, but keeps the JS changes

Use snakecase for php vars

2aed927

Add bottom padding all the time when floating

40f8977

This adds space between sidebar and footer when main content is short

adamwoodnz merged commit 09d0b33 into trunk Jan 31, 2024
2 checks passed

adamwoodnz deleted the enhance/scrolling-sidebar branch January 31, 2024 23:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sidebar container scroll internally #554

Make sidebar container scroll internally #554

adamwoodnz commented Jan 10, 2024 •

edited

Loading

renintw commented Jan 22, 2024

renintw left a comment

renintw Jan 22, 2024

ryelle Jan 23, 2024

adamwoodnz Jan 24, 2024

ryelle Jan 24, 2024

adamwoodnz commented Jan 24, 2024

renintw commented Jan 25, 2024

adamwoodnz commented Jan 30, 2024

adamwoodnz Jan 30, 2024

adamwoodnz Jan 30, 2024

ryelle left a comment

ryelle Jan 31, 2024

adamwoodnz Jan 31, 2024

ryelle Jan 31, 2024

adamwoodnz Jan 31, 2024

	$inlineBreakpoint = $attributes['inlineBreakpoint'];
	$inline_breakpoint = $attributes['inlineBreakpoint'];

Make sidebar container scroll internally #554

Make sidebar container scroll internally #554

Conversation

adamwoodnz commented Jan 10, 2024 • edited Loading

Testing

Things to test

renintw commented Jan 22, 2024

renintw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamwoodnz commented Jan 24, 2024

renintw commented Jan 25, 2024

adamwoodnz commented Jan 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryelle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamwoodnz commented Jan 10, 2024 •

edited

Loading