Repository navigation
fix(nav): elevate the navbar when the drawer is open - #89
Merged
Merged
Conversation
Painting the nav in the section's own tone (paper on paper, ink on ink) gave it zero contrast against the section behind it once the drawer opened, so the band read as no nav at all — just a wordmark floating above a dimmed page. Give the drawer-open nav real visual presence: - z-index lifted to 58 (above the overlay's 55, below the panel's 60) - backdrop-filter cleared so the bar is fully opaque - background painted in the section's own tone (var(--paper) or var(--ink) per data-tone), not flipped to match the drawer - hairline border-bottom matched to the scrolled-state values (24% ink on paper, 26% paper on ink) - soft drop shadow that elevates the band above the dimmed section (ink shadow on paper sections, paper glow on ink sections) - box-shadow added to the chrome-nav transition list so the shadow fades in alongside the drawer's open animation A new data-mobile-open attribute on the nav element drives the rule; Chrome.tsx reads mobileOpen from MobileMenu's onOpenChange callback (already wired). Tone keeps tracking the section behind the nav — the drawer is the paper anchor, the nav remains a continuation of the section.
Deploying website with
|
| Latest commit: |
c6936db
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5656aca2.website-70y.pages.dev |
| Branch Preview URL: | https://fix-nav-drawer-open-elevatio.website-70y.pages.dev |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the navigation bar's styling when the mobile menu is open by adding a data-mobile-open attribute and corresponding CSS rules for opaque backgrounds and elevation shadows. The feedback identifies redundant CSS selectors in the new styles, suggesting that the [data-scrolled="true"] attribute is unnecessary when the mobile menu is active.
There was a problem hiding this comment.
Code Review
This pull request updates the navigation bar's appearance and behavior when the mobile drawer is open. It introduces a data-mobile-open attribute to the navigation component, allowing for specific styling that removes translucent backdrops in favor of opaque backgrounds with elevated box shadows. Feedback suggests ensuring the new box-shadow transition is included in the prefers-reduced-motion media query for consistency and simplifying CSS selectors by removing redundant data-scrolled attributes when the drawer is open.
| backdrop-filter 260ms cubic-bezier(0.2, 0.8, 0.2, 1), | ||
| -webkit-backdrop-filter 260ms cubic-bezier(0.2, 0.8, 0.2, 1), | ||
| border-bottom-color 260ms cubic-bezier(0.2, 0.8, 0.2, 1), | ||
| box-shadow 360ms cubic-bezier(0.2, 0.8, 0.2, 1), |
There was a problem hiding this comment.
The box-shadow transition is added here, but it appears to be missing from the prefers-reduced-motion override for .chrome-nav (located later in the file around line 842). This will cause the shadow to snap instantly for users with reduced motion enabled, which contradicts the PR description's claim that shadow transitions 'complete cleanly'. Consider adding box-shadow 360ms ... to that block to ensure a smooth fade-in for all users.
Collaborator
Author
There was a problem hiding this comment.
Fixed in 0a29a6a — added box-shadow 360ms to the reduced-motion .chrome-nav transition list (alongside background-color). The PR-description claim was wrong as written; the override at the bottom of the file was indeed gutting the shadow under reduced-motion.
There was a problem hiding this comment.
Pull request overview
Adjusts the site chrome so the fixed navbar remains visually distinct and properly layered when the mobile drawer is open, while still tracking the active section tone.
Changes:
- Adds a
data-mobile-openattribute to the primary<nav>so CSS can target the drawer-open visual state. - Updates
.chrome-navtransitions to includebox-shadowfor smoother drawer-open elevation cues. - Introduces drawer-open navbar styling (higher z-index than overlay, opaque tone-matched background, hairline, and shadow).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| components/site/Chrome.tsx | Exposes drawer-open state via data-mobile-open on the navbar. |
| app/globals.css | Adds drawer-open navbar styling and transitions to keep the nav visually present above the overlay. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
712
to
718
| transition: | ||
| background-color 260ms cubic-bezier(0.2, 0.8, 0.2, 1), | ||
| backdrop-filter 260ms cubic-bezier(0.2, 0.8, 0.2, 1), | ||
| -webkit-backdrop-filter 260ms cubic-bezier(0.2, 0.8, 0.2, 1), | ||
| border-bottom-color 260ms cubic-bezier(0.2, 0.8, 0.2, 1), | ||
| box-shadow 360ms cubic-bezier(0.2, 0.8, 0.2, 1), | ||
| color 300ms cubic-bezier(0.2, 0.8, 0.2, 1); |
Two findings from the PR #89 bot reviews: 1. The `[data-scrolled="true"]` second selector on both per-tone drawer-open rules was redundant. The base `.chrome-nav[data-mobile-open="true"][data-tone="paper"]` selector (0,3,0) matches both scrolled and unscrolled states, and is later in source than the scrolled-state rules (also 0,3,0) — source order alone makes it win. Dropped the doubled selectors. 2. The `prefers-reduced-motion` block overrides `.chrome-nav`'s transition list down to just `background-color`, which silently undid the new `box-shadow 360ms` for users with reduced motion enabled (the shadow would snap instead of fade). Added `box-shadow` to the reduced-motion transition list so the fade completes for everyone — consistent with the rest of the file's pattern of keeping soft opacity-like transitions under reduced-motion (drawer panel keeps a 240ms opacity fade, etc.).
The close-effect cleanup unpinned the body (page snaps to y=0 because the negative top was the only thing holding the viewport) and then called window.scrollTo(0, scrollYRef.current). Since <html> carries motion-safe:scroll-smooth, that scrollTo defaulted to a smooth animation — so the user saw the page leap to the top and then scroll down to their prior position. Pass behavior: "instant" on the restore scrollTo so it bypasses the page's smooth-scroll CSS. The y=0 intermediate now happens in the same synchronous tick as the restore — no paint between, no animation. Close-only (×, overlay, ESC) stays exactly where the user was; anchor taps become a clean smooth ride from prior position to target instead of a jump-then-descent. Also drops the now-redundant else branch (the unconditional scrollTo above already restores the position; the anchor branch just rides on top of it).
Two correctness issues plus a comment-tightening pass surfaced during PR #89 review. Correctness ----------- - behavior: "instant" on scrollTo was silently degrading to "auto" on Safari < 18.4 — and "auto" honours <html>'s motion-safe:scroll-smooth, which re-introduces the jump-to-top the previous commit was meant to kill. Swap the page's scroll-behavior to "auto" around a legacy two-arg scrollTo instead; that works regardless of how the "instant" enum is interpreted. - The scroll restore was unconditional, so when the drawer was opened from /blog/<slug> and the user tapped a section row, the blog page (which is about to unload) would visibly scroll back to scrollYRef.current before window.location.assign navigated. Now we only restore when staying on this page — anchor with the target found, or no anchor at all. Comments -------- - Trim the cleanup-block lead (10 lines → mechanism-only one-liner plus the Safari note where the WHY actually lives). - Drop the duplicated framing in the toggleTone comment in Chrome.tsx (the data-mobile-open behaviour is documented at its CSS site, doesn't need restating here). - Compress the globals.css drawer-open lead block; the contrast-failure WHY is the only load-bearing sentence and now stands alone.
- The drawer's section rows render in lowercase (`compare`, `method`, `faq`, `contact`), but the external list below them was Title-Cased (`Docs`, `Blog`, `Litepaper`). Match the editorial- lowercase vocabulary. - The footer read `decdn / labs` → `DECDN / LABS` via `.meta`'s uppercase. Swap the slash for the wordmark's underscore and paint it whisper-green so `DECDN_LABS` echoes the wordmark mark.
Collaborator
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves the mobile navigation bar's visual transitions and state management, specifically handling the drawer-open state with new CSS styles and data attributes. It also refactors scroll restoration logic in the MobileMenu component to fix Safari-specific scrolling issues and updates several UI labels. A review comment suggests replacing window.location.assign with Next.js's router.push for internal navigation to prevent unnecessary full page reloads and preserve application state.
Three of four bot suggestions verified valid and applied: - Cross-route navigation goes through next/navigation's router.push instead of window.location.assign — keeps the /blog/* -> / hop client-side, matches the <Link>-based section nav in Chrome.tsx. router added to the open-effect deps (stable, so harmless). - The decorative underscore in the footer (decdn_labs) gets aria-hidden so screen readers no longer announce "underscore" — the accessible name is now "decdn labs", a clean wordmark echo without an audible artefact. - border-bottom-color added to the prefers-reduced-motion .chrome-nav transition list. Matches the existing pattern of keeping color-only transitions under reduced-motion; without it the hairline snapped while the background + shadow faded. The fourth bot suggestion (source-case mismatch between desktop "Docs"/"Blog"/"Litepaper" and the drawer's "docs"/"blog"/ "litepaper") was rejected — the visual output is intentional: .meta carries text-transform: uppercase so the desktop renders uppercase regardless of source case, while .mm-label in the drawer renders verbatim. Source-case difference is cosmetic only.
…-expo scrollIntoView's CSS smooth-scroll was completing in ~300-500ms, which read as snappy next to the drawer's 720ms slide and 850ms brutal-rise reveals — anchor taps felt clipped against the rest of the page's editorial choreography. Replace it with a rAF loop running 900ms easeOutExpo (1 - 2^-10t), matching the heavy settling-into-place feel the rest of the page uses. Reduced-motion path keeps the existing scrollIntoView (instant under prefers-reduced-motion), preserves scroll-margin-top via getComputedStyle so future --nav-h changes work without touching this helper. Drawer slide-close (720ms) and the scroll (900ms) now overlap such that the drawer dismisses first and the scroll's tail-end remains visible — clean visual hierarchy that reads as one coordinated motion rather than two competing animations.
The 900ms easeOutExpo from d86794b covered ~50% of the distance in the first 10% of time and then dragged out the rest — combined with the drawer's 720ms slide-close occluding the high-velocity opening burst, this read as "slow at first, then suddenly speeds up." Constant velocity at 600ms reads as deliberate without any internal pace shift. scrollToAnchor's eased term collapses to just `t`, duration drops from 900 to 600. Reduced-motion path unchanged (scrollIntoView under prefers-reduced-motion still jumps instantly). Comments above the helper and at the call site updated to match.
The previous linear-rAF scroll still felt lumpy ("slow at first,
then suddenly speeds up"). Cause: after the cleanup's instant
restore, `<html>`'s scrollBehavior reverts to "" and Tailwind's
`motion-safe:scroll-smooth` kicks back in. Each per-frame
window.scrollTo inside scrollToAnchor then defers to CSS
scroll-behavior: smooth, so the browser queues a new managed
smooth animation toward a moving target every tick — the rAF and
the CSS smooth-scroll fight each other and the perceived velocity
goes uneven.
Have scrollToAnchor own its own swap: force scrollBehavior to
"auto" before the first frame, restore the previous value when
the loop completes. Each per-frame scrollTo now lands instantly,
which is what the rAF assumes. The result is genuinely constant
pixels-per-frame.
Two correctness fixes plus a comment-tightening pass surfaced by the post-50878bc review. Critical -------- - scrollToAnchor is now single-flight. Module-scope state (scrollAnchorRaf + scrollAnchorRestore) tracks the in-flight rAF id and the original prevScrollBehavior. A new call cancels the prior rAF and eagerly restores its captured prev BEFORE snapshotting again, so the polluted "auto" value can never become the captured "previous" state. Without this, two overlapping calls could permanently strand <html> at scroll-behavior: auto for the rest of the session. Important --------- - Reverted the cross-route gate from router.push to window.location.assign. Next 16 App Router's hash-on-soft-nav behaviour under output: "export" isn't documented; full reload is deterministic. Drops useRouter + the router dep on the open-effect. Suggestions ----------- - Updated the portal-target comment to reflect that the nav now rides z-50 resting / z-58 while the drawer is open (was stale after the data-mobile-open block landed in globals.css). - Trimmed scrollToAnchor's doc-comment (WHY-only), the cross- route gate comment, and the replaceState comment. - Dropped dead-defense `|| 0` after parseFloat — getComputedStyle always returns a pixel string for an unset property.
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.
Summary
Four improvements to the mobile drawer that all surfaced after PR #56 landed.
z-55) dimming the strip, the wordmark appeared to float on a dim page with no surrounding header.window.scrollTo, which<html>'smotion-safe:scroll-smooththen animated fromy=0(where unpinning the body left the viewport) back up to the saved position — reading as a jump to the top and a smooth descent.Docs / Blog / Litepaper) while the section rows above it were lowercase — inconsistent. The footer readdecdn / labsinstead of echoing the wordmark's underscore mark.<html>'s CSS scroll-smooth (each per-framescrollToqueued another browser smooth animation toward a moving target, producing lumpy velocity). The final answer is 600ms linear rAF with<html>scroll-behaviorforced toautofor the loop's lifetime, single-flight so overlapping taps can't leak the override past the helper.This PR fixes all four: nav stays as a continuation of the section behind it (no white-on-black flip), closes/anchor-taps land without a visible smooth-scroll animation from
y=0, the external list + footer match the brand's editorial-lowercase + wordmark vocabulary, and the section scroll runs at a deliberate-but-not-sluggish constant pace.Notable mechanics
Nav elevation
data-mobile-openon<nav>(driven by themobileOpenstate already wired throughMobileMenu'sonOpenChange) gives the CSS a single tight selector for the drawer-open visual state, without overloadingdata-tone.z-index: 58lifts the nav above the overlay'sz-55but leaves it below the panel'sz-60, so the panel still covers most of the nav from the right and the overlay no longer dims the visible left strip.var(--paper)on paper sections,var(--ink)on ink) — section content can't bleed through the visible strip.box-shadowandborder-bottom-coloradded to both the base andprefers-reduced-motiontransition lists so the shadow and hairline fade alongside the drawer's open animation under both motion preferences.Scroll-restore on close
<html>scroll-behaviortemporarily swapped toautoaround a synchronous two-argscrollTo(0, scrollYRef)in the close-effect cleanup. Doesn't usebehavior: "instant"because Safari only honoured the"instant"enum from 18.4; older versions silently fell back to"auto"which still respected CSS smooth-scroll — i.e. the original bug. The CSS-swap pattern is universally synchronous regardless of how the enum is interpreted./blog/<slug>and the user taps a section row, the local restore is skipped andwindow.location.assign('/#section')is used so the new page handles the hash scroll deterministically — Next 16 App Router's hash-on-soft-nav underoutput: "export"isn't documented; a full reload is the safe bet.Section-anchor scroll
scrollToAnchorhelper at module level runs a rAF loop at constant velocity (t, not eased) over 600ms — slower than the browser default but linear, so there's no high-velocity burst followed by a slow tail.<html>scroll-behaviorswap (setsautobefore the first frame, restores at completion). Without this, per-framewindow.scrollTodefers tomotion-safe:scroll-smoothand queues a fresh browser smooth-scroll every tick, fighting the rAF and producing visibly uneven velocity.prevScrollBehaviorbefore snapshotting a fresh one. Prevents the failure mode where overlapping calls capture the already-polluted"auto"as "previous" and strand<html>permanently atscroll-behavior: auto.scroll-margin-topviagetComputedStyle, so the existingFrame'sscroll-mt-[var(--nav-h)]keeps working without changes.scrollIntoView({ block: "start" }), which honoursprefers-reduced-motionautomatically (jumps instantly).Typography polish
EXTERNALlabels lowercased todocs / blog / litepaper, matching the editorial-lowercase section rows above them in the drawer. Desktop right cluster keeps Title-Case source —.meta'stext-transform: uppercasemakes the rendered output identical either way.decdn / labs→decdn_labswith the_paintedtext-whisperandaria-hiddenso screen readers announce "decdn labs" without an "underscore" artefact..metauppercases and tracks the source toD E C D N _ L A B S, with just the underscore highlighted in green — echoing the wordmark's brand mark instead of an arbitrary slash separator.Test plan
Nav elevation
<mdviewport, open the drawer onintro(paper section): nav reads as a paper band with a 24% ink hairline and a soft ink shadow underneath, distinct from the dimmed page below.compareorfaq(ink sections): nav reads as a black band with a 26% paper hairline and a paper glow underneath. Tone is NOT inverted to match the drawer.data-mobile-openclears, nav reverts to its section-tracked behaviour.prefers-reduced-motion: reduce): nav background, border, and shadow still transition (theprefers-reduced-motionblock's transition list now includesborder-bottom-colorandbox-shadow); other chrome transitions remain disabled.Scroll-restore on close
/: page stays exactly at the pre-open scroll position. Zero visible scroll./: drawer closes and the page rides from the prior position to the target at constant velocity over 600ms./blog/<slug>: full reload to/#section; the new page handles the hash scroll on load.<html>smooth-scroll setting — the CSS swap kicks in and the legacy two-argscrollTolands instantly.Section-anchor scroll
/: scroll is linear (constant pixels-per-millisecond) over 600ms — no perceptible easing.<html>'sscroll-behavior(verify via DevTools thathtml.style.scrollBehaviorreturns to""after the second scroll finishes).Typography polish
docs,blog,litepaper(lowercase, same visual weight as the section rows).DECDN_LABSwith the_character rendered in whisper green; letter-spacing applies uniformly across all characters incl. the underscore._span carriesaria-hidden.🤖 Generated with Claude Code