chore: theme token cleanup + chrome-active helper#137
Conversation
The `--font-sans` entry in `@theme inline` was never read: `html, body` applies `font-family: var(--font-geist-sans), …` directly, and no component uses the `font-sans` utility class (verified by grep across `.ts(x)` / `.mdx`). Removing it loses no behavior — the `font-sans` utility falls back to Tailwind's default sans stack — and shrinks the theme surface. Closes #105 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Finishes the migration left at the end of #78 / #101: the typography half landed under the `--text-*` namespace (Approach A); the layout half stayed as raw vars consumed via `px-[var(--frame-gutter)]` / `max-w-[var(--frame-max)]` arbitrary-value classes (Approach B). Renames so layout tokens auto-generate utilities: - `--frame-gutter` → `--spacing-frame-gutter` ⇒ `px-frame-gutter` - `--frame-pad-y` → `--spacing-frame-pad-y` ⇒ `py-frame-pad-y` - `--frame-max` → `--container-frame` ⇒ `max-w-frame` - `--frame-min-h-cap` → `--spacing-frame-min-h: min(100svh, 54rem)` ⇒ `min-h-frame-min-h` (the `min()` was already at the lone call site; folding it into the token removes the last arbitrary value) Frame.tsx, Footer.tsx, and Chrome.tsx's <nav> switch to the generated utilities. The mobile-menu close button's raw CSS ref becomes `right: var(--spacing-frame-gutter)`. The `--nav-h` comment block is updated to match. Closes #102 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The home-route-gated nav-tone ternary in `Chrome.tsx` (the #112 fix plus the back-nav flash guard) was load-bearing but uncovered. Pulling it into a pure helper lets it be unit-tested without dragging in jsdom / RTL / IntersectionObserver mocks — the function takes `(pathname, scrolled, observed, fallback)` and is independent of any runtime. Adds five cases from the issue, including the two regressions the original fix exists to prevent: off-home stale `active` (the #112 bug) and back-nav pre-scroll flash. `vitest.config.ts`'s existing `**/*.test.ts` include glob picks the new file up unchanged. Closes #117 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Deploying website with
|
| Latest commit: |
86293ff
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://376b8da6.website-70y.pages.dev |
| Branch Preview URL: | https://chore-theme-chrome-followups.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 refactors CSS variable naming to align with Tailwind CSS v4 conventions and extracts the active section resolution logic into a dedicated utility function with accompanying tests. Key changes include renaming frame-related spacing and sizing variables and updating the Chrome, Footer, and Frame components to use these new utility classes. A critical issue was identified regarding the use of the --container-* namespace in app/globals.css, which does not automatically generate the max-w-frame utility; it is recommended to use the --width- namespace instead to ensure the styles are correctly applied.
| --frame-gutter: clamp(1.5rem, 4vw, 4rem); | ||
| --frame-pad-y: clamp( | ||
| /* Shared content rail: every Frame centers within --container-frame. */ | ||
| --container-frame: 1280px; |
There was a problem hiding this comment.
In Tailwind CSS v4, the --container-* namespace is specifically reserved for configuring the container utility (e.g., centering, padding, or breakpoint-specific max-widths). It does not automatically generate a max-w-* utility for the given name. To enable the max-w-frame utility used in Chrome.tsx, Footer.tsx, and Frame.tsx, you should use the --width-* namespace (or --spacing-* if you want it available for padding/margin as well).
--width-frame: 1280px;There was a problem hiding this comment.
Verified — --container-* does generate max-w-* in Tailwind v4. After pnpm build on this branch (39ac7c8), the static export contains .max-w-frame{max-width:1280px} — exactly the utility Chrome.tsx / Footer.tsx / Frame.tsx rely on. Tailwind v4's own defaults follow the same pattern: node_modules/tailwindcss/theme.css defines --container-3xs … --container-7xl, which back the framework's max-w-3xs … max-w-7xl utilities, and the official v4 docs implement every max-w-* as max-width: var(--container-<name>) (e.g. max-w-2xs is max-width: var(--container-2xs)).
The @frame: container-query variant the comment alludes to is an additional output of --container-*, not a substitute — both happen. --width-* isn't a max-width-generating namespace in v4, so renaming to --width-frame would actually break max-w-frame. No change needed.
There was a problem hiding this comment.
Pull request overview
Chore PR that cleans up theme tokens and adds a small testable helper. Removes an unreferenced --font-sans token, promotes --frame-* layout vars to Tailwind v4 namespaces (--spacing-frame-* / --container-frame) so plain utilities like px-frame-gutter / max-w-frame replace arbitrary-value classes, and extracts the home-route nav-tone ternary in Chrome.tsx into a pure resolveActiveSection helper with 5 unit tests.
Changes:
- Drop unreferenced
--font-sansand rename--frame-*tokens to Tailwind v4 namespaces; update all consumers and the lone CSS ref. - Replace
[var(--frame-*)]arbitrary-value classes inFrame,Footer, andChromewith generated utilities. - Extract
resolveActiveSectionhelper fromChrome.tsxand add table-driven Vitest cases.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| app/globals.css | Removes --font-sans; renames --frame-* → --spacing-frame-* / --container-frame; folds min() into --spacing-frame-min-h; updates comments and .mm-toggle right. |
| components/ui/Frame.tsx | Switches to px-frame-gutter py-frame-pad-y, min-h-frame-min-h, and max-w-frame utilities. |
| components/site/Footer.tsx | Switches to px-frame-gutter / max-w-frame utilities. |
| components/site/Chrome.tsx | Switches nav classes to new utilities; replaces inline ternary with resolveActiveSection(...). |
| components/site/chrome-active.ts | New pure helper returning observed when on / and scrolled, else fallback. |
| components/site/chrome-active.test.ts | Adds 5 Vitest cases covering the #112 bug, back-nav flash guard, passthrough, and off-home dominance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The "(the #112 bug)" parenthetical belongs in PR / commit history, not in a long-lived test name where it rots once the issue is forgotten. The behavior the test guards (off-home + scrolled → fall back) is already self-describing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…h test Addresses review feedback on the helper introduced in 39ac7c8: - Constrain `<T extends string>` so unrelated types can't unify silently (`resolveActiveSection(p, s, 42, "intro")` stops compiling). Preserves the literal-union inference at the call site. - Move the multi-line guard rationale from above the call site in Chrome.tsx to above the function in chrome-active.ts, where the `pathname === "/" && scrolled` logic actually lives. The original framing leaked Chrome.tsx-specific vocabulary (`DARK_SECTIONS`, `text-paper`); tightened to describe the helper's own contract. - Add a 6th vitest case asserting the helper is exact-match (`/?utm=x` falls back), defending against a future caller that swaps usePathname() for something query-/hash-including. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Walks back two refinements from 0730f82: - The constraint blocked no real misuse — the sole call site already infers T to a literal-string union from the caller's typing. - The rehomed comment said the off-home observer "holds stale values" but the observer is never built off-home (Chrome.tsx's IO useEffect early-returns when no section nodes exist). It also kept the inherited "past the intro hero" framing for `scrolled`, which is misleading — scrolled flips at scrollY > 8, well inside intro. Rewritten as the "new observer has had a moment to fire" heuristic it actually is, with the back-nav restored-scroll caveat noted. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Summary
--font-sanstoken from@theme inline—html, bodyalready applies the typeface directly and no component uses thefont-sansutility (closes chore(theme): decide on the unreferenced --font-sans token #105).--frame-*layout tokens to Tailwind v4 namespaces (--spacing-frame-*/--container-frame), sopx-frame-gutter/py-frame-pad-y/max-w-frame/min-h-frame-min-hutilities replace the[var(--frame-*)]arbitrary-value classes left over from chore(theme): migrate inline layout/typography CSS-var styles to Tailwind #101. Finishes the Approach-A migration started in chore(theme): migrate inline layout/typography CSS-var styles to Tailwind #78 (closes chore(theme): promote --frame-* layout tokens to Tailwind namespaces #102).Chrome.tsxinto a pureresolveActiveSectionhelper (rationale doc co-located with the function) and add 6 unit tests covering both regression guards, all four AND-gate quadrants, and the exact-match boundary — no jsdom needed (closes chrome: extract effectiveActive into pure helper + add unit test #117).#115 was reviewed in parallel and intentionally deferred (audit comment posted on the issue); the helper extracted here is where its routing-aware fix will live.
Test plan
pnpm lint— cleanpnpm exec tsc --noEmit— cleanpnpm test— 101 pass, including the 6resolveActiveSectioncasespnpm build— static export clean; built CSS contains.px-frame-gutter/.py-frame-pad-y/.max-w-frame/.min-h-frame-min-hand zerovar(--frame-*)refspnpm devsmoke: gutter / max-width rail / section min-height visually unchanged on/,/blog/, and a post/→text-paper; back-nav/blog/→/paintstext-inkuntil first scroll;/blog/foo/staystext-inkregardless of scroll×sits at the same gutter offset🤖 Generated with Claude Code