fix: prevent sidebar jitter, scroll passthrough, and nav z-index overlap#119
fix: prevent sidebar jitter, scroll passthrough, and nav z-index overlap#119opentabs-dev wants to merge 1 commit intoLogging-Studio:mainfrom
Conversation
Three related bugs fixed: 1. Sidebar jitter on scroll sticky + self-start in a flex container causes 1-2px drift each frame as the browser recalculates position. Fixed by making the sidebar fixed so it is composited independently of page scroll. left tracks the max-w-7xl centered container via max(0px, (100vw - 80rem) / 2). 2. Scroll passthrough on sidebar and TOC overflow-y-auto only intercepts wheel events when content overflows. Hovering over gaps between items or on a short sidebar sends scroll to the page instead. Fixed with overflow-y-scroll (always a scroll container) and overscroll-y-contain (no chaining at boundaries). sidebar-scroll already hides the forced scrollbar track visually. 3. TopNav z-index too low (z-1) Page content with higher z-index (copy buttons, etc.) rendered over the sticky nav while scrolling. Fixed by raising nav to z-50. Files changed: - app/(docs)/docs/layout.tsx - components/SideNav.tsx - components/TableOfContents.tsx - components/TopNav.tsx
|
@opentabs-dev is attempting to deploy a commit to the Retro UI Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe documentation layout transitions from a responsive in-flow sidebar to a fixed 60px left sidebar with centered content area. Accompanying changes simplify the sidebar animation behavior, adjust overflow and scrolling properties in the table of contents, and increase the top navigation's z-index stacking order for proper layering. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/(docs)/docs/layout.tsx (1)
17-17: Hardcoded80remduplicates themax-w-7xltoken.The
calc((100vw - 80rem) / 2)in theleftoffset assumesmax-w-7xlis 80rem. If the Tailwind config ever customizes that breakpoint, this value would silently drift. Consider extracting a CSS custom property (e.g.,--container-max: 80rem) used by both the container and the offset, or leave a comment noting the coupling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/`(docs)/docs/layout.tsx at line 17, The hardcoded "80rem" in the lg:left calc inside the div class (the element with className "fixed top-28 hidden h-[calc(100vh-7rem)] w-60 lg:left-[max(0px,calc((100vw-80rem)/2))] lg:block") duplicates the max-w-7xl token and can drift if Tailwind config changes; replace the literal with a CSS custom property (for example --container-max) used by both the container and this offset (update the container element that uses max-w-7xl to set --container-max: 80rem or the shared value) or, if you cannot change the container, add a clear comment next to the lg:left usage noting the coupling so future maintainers know why 80rem is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/`(docs)/docs/layout.tsx:
- Line 17: The fixed sidebar in layout.tsx uses a hardcoded "top-28" value which
can mismatch the actual TopNav height; replace the hardcoded top with a dynamic
value by exposing the TopNav height as a CSS variable or measured value and
using that for the sidebar top. Specifically, have the TopNav component set
--topnav-height (or expose its clientHeight via a ref/useEffect) and update the
sidebar div currently using "top-28" to use top: var(--topnav-height) (or inline
style from the measured value) so the sidebar always aligns without gap/overlap;
then test around the lg breakpoint where the promo banner wraps.
---
Nitpick comments:
In `@app/`(docs)/docs/layout.tsx:
- Line 17: The hardcoded "80rem" in the lg:left calc inside the div class (the
element with className "fixed top-28 hidden h-[calc(100vh-7rem)] w-60
lg:left-[max(0px,calc((100vw-80rem)/2))] lg:block") duplicates the max-w-7xl
token and can drift if Tailwind config changes; replace the literal with a CSS
custom property (for example --container-max) used by both the container and
this offset (update the container element that uses max-w-7xl to set
--container-max: 80rem or the shared value) or, if you cannot change the
container, add a clear comment next to the lg:left usage noting the coupling so
future maintainers know why 80rem is required.
| <div className="mx-auto max-w-7xl"> | ||
| {/* Sidebar — fixed to the viewport, left-aligned with the max-w-7xl (80rem) | ||
| centered container. On screens narrower than 80rem, left is 0. */} | ||
| <div className="fixed top-28 hidden h-[calc(100vh-7rem)] w-60 lg:left-[max(0px,calc((100vw-80rem)/2))] lg:block"> |
There was a problem hiding this comment.
top-28 (7rem) is coupled to the TopNav's rendered height — verify no gap or overlap.
The TopNav has a variable-height promotional banner + h-16 nav bar + 2px border. If the banner text wraps (e.g., on a screen just past the lg breakpoint), the total height could exceed 7rem, causing the sidebar to tuck under the nav. Conversely, if the banner is shorter than expected, there's a visible gap.
Since you've verified locally, this is likely fine, but it's worth testing at viewport widths near 1024px where the sidebar first appears and the banner is most likely to wrap.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/`(docs)/docs/layout.tsx at line 17, The fixed sidebar in layout.tsx uses
a hardcoded "top-28" value which can mismatch the actual TopNav height; replace
the hardcoded top with a dynamic value by exposing the TopNav height as a CSS
variable or measured value and using that for the sidebar top. Specifically,
have the TopNav component set --topnav-height (or expose its clientHeight via a
ref/useEffect) and update the sidebar div currently using "top-28" to use top:
var(--topnav-height) (or inline style from the measured value) so the sidebar
always aligns without gap/overlap; then test around the lg breakpoint where the
promo banner wraps.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
not sure about the issue though. Can you share a before and after screenshot? |
Sure, i think i will do a screenrecording. Also could you delete this #118 (comment) comment? thank you |
2 participants
Summary
This PR fixes three bugs in the docs layout that affect scrolling behaviour and visual layering.
Bug 1 — Sidebar jitter while scrolling
Problem: The sidebar uses
position: stickywithself-startinside a flex container. Becausestickyelements still participate in layout flow, the browser recalculates their position on every scroll frame. This causes a 1–2px drift before the element is re-pinned, visible as a subtle jitter.Fix: Replace
stickywithfixedpositioning on the sidebar wrapper. Afixedelement is composited independently of page scroll — it never moves. Theleftoffset usesmax(0px, calc((100vw - 80rem) / 2))so the sidebar stays aligned with themax-w-7xlcentered container on wide screens.Bug 2 — Scroll events pass through sidebar and TOC to the page
Problem: Both the sidebar and the "On this Page" TOC box use
overflow-y-auto. This only creates a scroll container when content overflows. When content is shorter than the container — or when the pointer is over a gap between items — the browser does not consider the element a scroll target and routes wheel events directly to the page, scrolling the main content instead.Fix: Replace
overflow-y-autowithoverflow-y-scrollon both elements. This forces them to always be scroll containers, so wheel events are always captured regardless of content height. Addoverscroll-y-containto prevent scroll chaining to the page when the element reaches its boundary. The existingsidebar-scrollCSS class already hides the forced scrollbar track visually — no visual regression.Bug 3 — TopNav
z-indextoo low, page content renders over itProblem: The
<nav>hasz-1. Any page content with a higherz-index(e.g. code block copy buttons) renders on top of the sticky nav while scrolling.Fix: Raise nav to
z-50. Also removed a stalez-99class from the inner<nav>inSideNavwhich had no effect.Files Changed
app/(docs)/docs/layout.tsx— fixed sidebar positioningcomponents/SideNav.tsx—overflow-y-scroll,overscroll-y-contain, removed stale z-indexcomponents/TableOfContents.tsx—overflow-y-scroll,overscroll-y-containcomponents/TopNav.tsx— raised z-index toz-50Testing
Tested locally with
pnpm dev. Verified:Summary by CodeRabbit