Keep the sidebar within the viewport - #476
Open
gfiguero wants to merge 1 commit into
Open
Conversation
The sidebar is a grid item spanning all four rows, so `block-size: 100%` resolves against a grid area as tall as the whole document. Its `overflow` never kicks in, and the absolutely positioned table of contents inside it stretches to the bottom of the page. In a long book that leaves the page scrollbar as the only way to reach the end of the table of contents, and scrolling the text drags the navigation out of view. Stick the sidebar to the viewport and bound it to `100dvh` so the table of contents scrolls its own content instead.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a regression test and adjusts sidebar CSS so the table of contents sidebar is constrained to the viewport and remains visible while the document scrolls.
Changes:
- Add a system test validating sidebar height is capped to the viewport and that it doesn’t scroll away.
- Update
#sidebarstyling to size and stick relative to the viewport (usingdvh+position: sticky).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/system/sidebar_scroll_test.rb | Adds system coverage for sidebar height/scroll behavior. |
| app/assets/stylesheets/layout.css | Switches sidebar sizing to viewport units and makes it sticky to prevent page-length growth. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+12
to
+13
| assert_operator document_height, :>, viewport_height * 2, | ||
| "the test page should be considerably taller than the viewport" |
| test "the table of contents stays put while the page scrolls" do | ||
| execute_script "window.scrollTo(0, document.documentElement.scrollHeight)" | ||
|
|
||
| assert_equal 0, sidebar_top.round, |
| :where(#sidebar) { | ||
| background-color: var(--color-subtle-light); | ||
| block-size: 100%; | ||
| block-size: 100dvh; |
Comment on lines
+105
to
+108
| inset-block-start: 0; | ||
| max-block-size: 100dvh; | ||
| overflow: auto; | ||
| position: relative; | ||
| position: sticky; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #475.
The problem
#sidebaris a grid item spanning all four rows of thebodygrid, soblock-size: 100%andmax-block-size: 100%resolve against a grid area as tall as the whole document. The element is never shorter than its content, so itsoverflow: autonever engages, and the absolutely positioned.toc.sidebar__contentinside it (inset: 0 auto 0 0) stretches to the bottom of the page along with it.In a book with long pages that leaves the page scrollbar as the only way to reach the end of the table of contents, and scrolling the text drags the navigation out of view. On a 1257px viewport with a long page the sidebar measures just over 20,000px tall.
The fix
Stick the sidebar to the viewport and bound it to
100dvh, so it scrolls its own content:stickyrather thanfixedkeeps the sidebar a grid item, so the column keeps its width and the open/closemargin-inline-starttransition is untouched. It also still establishes the containing block for the absolutely positioned menu inside it.dvhneeds a 2022-or-newer browser, which is no newer than the:has()and container queries already in use. The sidebar isdisplay: nonebelow70ch, so the mobile layout is unaffected.Tests
test/system/sidebar_scroll_test.rbcovers both halves of the problem, and both fail without the change:20970.64pxin a1257pxviewporttop: 0after the page is scrolled to the bottomFull suite is green on Ruby 3.4.7.