feat(tokens): add --sf-density control-geometry dial - #615
Conversation
…ywhere --sf-density (PUBLIC-ADVANCED, default 1) is the compact <-> comfortable dial for interactive control geometry. It scales the --sf-size-* rung ladder (24/32/40/48/56px at 1); below 1 packs controls tighter (dashboards, data tables, power-user tools), above 1 loosens them. The size ladder was previously static rem literals scaled by no knob at all — intentionally non-fluid because control height must not shrink on small screens where touch targets need to grow. Density keeps that non-fluid contract but gives the ladder one deliberate dial. Orthogonal to --sf-space-scale (whitespace) and --sf-section-scale (section rhythm); combine for a fully compact UI. The --sf-touch-target a11y floor stays independent, so a small value can't pull native controls under the WCAG target. Like the other global multipliers it is a :root dial (the ladder is computed at :root and inherits). Wiring: core/tokens.css (token + re-pointed --sf-size-*), token-tiers.js (PUBLIC-ADVANCED), token-annotations, llm-guide (global-multipliers section), configurator domain-patterns (spacing), demo override generator, token snapshot, static coverage artifacts, generated indexes; new tokens.spec test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TsAowhRHNTVVfxvafVtDas
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds the public-advanced ChangesDensity token integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Configurator
participant RootTokens
participant SizeLadder
participant Playwright
Configurator->>RootTokens: Recognize and set --sf-density
RootTokens->>SizeLadder: Scale --sf-size-* values
Playwright->>SizeLadder: Measure computed rung sizes
SizeLadder-->>Playwright: Return density-scaled pixels
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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)
tests/tokens.spec.js (1)
349-380: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid shadowing the top-level
measurehelper.This local
measurefunction shadows the top-levelmeasurehelper defined at line 71. Consider renaming it to something likemeasureSizesto prevent confusion and improve readability.♻️ Proposed refactor
- const measure = () => page.evaluate(() => { + const measureSizes = () => page.evaluate(() => { const rungs = ['xs', 's', 'm', 'l', 'xl']; const read = () => Object.fromEntries(rungs.map((r) => { const el = document.createElement('div'); el.style.blockSize = `var(--sf-size-${r})`; document.body.appendChild(el); const px = parseFloat(getComputedStyle(el).blockSize); el.remove(); return [r, px]; })); return read(); }); - const base = await measure(); + const base = await measureSizes(); // Default ladder: 24 · 32 · 40 · 48 · 56 px. expect(base.xs).toBeCloseTo(24, 0); expect(base.m).toBeCloseTo(40, 0); expect(base.xl).toBeCloseTo(56, 0); // Compact: every rung shrinks by the same factor — ladder preserved, not flattened. await page.evaluate(() => document.documentElement.style.setProperty('--sf-density', '0.8')); - const compact = await measure(); + const compact = await measureSizes(); for (const r of ['xs', 's', 'm', 'l', 'xl']) { expect(compact[r] / base[r], `${r} ×density`).toBeCloseTo(0.8, 2); } // Roomy: same knob loosens the ladder. await page.evaluate(() => document.documentElement.style.setProperty('--sf-density', '1.25')); - const roomy = await measure(); + const roomy = await measureSizes(); expect(roomy.m / base.m).toBeCloseTo(1.25, 2);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/tokens.spec.js` around lines 349 - 380, Rename the local measure helper in the density ladder test to a distinct name such as measureSizes, and update its base, compact, and roomy invocations accordingly; leave the top-level measure helper unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@reports/full-api-audit/results/tokens-report.json`:
- Around line 3-4: Regenerate the tokens report using the existing
audit-generation script so all --sf-size-* entries reflect the current source of
truth, including the consumption role and density-scaled calc() values, not the
stale knob/rem values.
---
Nitpick comments:
In `@tests/tokens.spec.js`:
- Around line 349-380: Rename the local measure helper in the density ladder
test to a distinct name such as measureSizes, and update its base, compact, and
roomy invocations accordingly; leave the top-level measure helper unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bc3e9d32-0a53-4a13-a65d-1b9e0c336162
⛔ Files ignored due to path filters (3)
configurator/src/data/api-index.generated.jsonis excluded by!**/*.generated.*configurator/src/data/token-registry.generated.jsonis excluded by!**/*.generated.*dist/css-custom-data.jsonis excluded by!**/dist/**
📒 Files selected for processing (21)
CHANGELOG.mdconfigurator/src/data/domain-patterns.jsoncore/tokens.cssdemos/full-api-demo-with-overrides.htmldemos/full-api-demo.htmldemos/generate.mjsdemos/ultimate-override.cssdocs/api-index.jsondocs/api-index.mddocs/llm-guide.mddocs/registry.jsondocs/test-coverage-6-token-reference.htmldocs/token-annotations.jsondocs/token-index.jsondocs/token-index.mddocs/tokens.mdreports/full-api-audit/results/tokens-report.jsonscripts/token-tiers.jstests/token-api.snapshot.jsontests/tokens.spec.jstoken-registry.json
Greptile SummaryAdds
Confidence Score: 4/5The CSS change is backwards-compatible and the default ladder resolves identically; the functional implementation is solid, but three documentation artifacts make an incorrect claim about how nested overrides work. The token wiring, tier classification, registry updates, demo changes, and test are all correct. The only substantive issue is that the block comment in core/tokens.css, the paragraph in docs/llm-guide.md, and the annotation in docs/token-annotations.json all assert that setting --sf-density on a nested element does not rescale the --sf-size-* tokens for that subtree — but CSS var() substitution is lazy, so it does. This misinformation would lead developers away from a genuinely useful component-level density pattern and could cause confusion when the real browser behaviour contradicts the docs. core/tokens.css (block comment lines 1192-1195), docs/llm-guide.md (lines 1219-1220), and docs/token-annotations.json (--sf-density annotation) — all carry the same incorrect nested-override claim and should be updated together. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["--sf-density (default: 1)"] --> B["--sf-size-xs = calc(1.5rem × density)"]
A --> C["--sf-size-s = calc(2rem × density)"]
A --> D["--sf-size-m = calc(2.5rem × density)"]
A --> E["--sf-size-l = calc(3rem × density)"]
A --> F["--sf-size-xl = calc(3.5rem × density)"]
G["--sf-touch-target (accessibility.css)"] -. "independent floor" .-> H[native controls]
B & C & D & E & F --> H
I["--sf-space-scale"] -. orthogonal .-> A
J["--sf-section-scale"] -. orthogonal .-> A
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["--sf-density (default: 1)"] --> B["--sf-size-xs = calc(1.5rem × density)"]
A --> C["--sf-size-s = calc(2rem × density)"]
A --> D["--sf-size-m = calc(2.5rem × density)"]
A --> E["--sf-size-l = calc(3rem × density)"]
A --> F["--sf-size-xl = calc(3.5rem × density)"]
G["--sf-touch-target (accessibility.css)"] -. "independent floor" .-> H[native controls]
B & C & D & E & F --> H
I["--sf-space-scale"] -. orthogonal .-> A
J["--sf-section-scale"] -. orthogonal .-> A
Reviews (1): Last reviewed commit: "feat(tokens): add --sf-density control-g..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| WCAG target. Like the other global multipliers it is a :root | ||
| dial — the size ladder is computed at :root and inherits, so a | ||
| nested override does not retroactively rescale it (same | ||
| computed-value rule as --sf-space-scale et al.). */ |
There was a problem hiding this comment.
The comment (and matching prose in docs/llm-guide.md and docs/token-annotations.json) states that a nested --sf-density override "does not retroactively rescale" the --sf-size-* ladder. This is incorrect. CSS custom property values are token streams that are substituted lazily at used-value time in each element's own cascade context — the same mechanism used by --sf-space-scale inline in the space tokens. A descendant element with --sf-density: 0.8 set on it inherits the token stream calc(2.5rem * var(--sf-density)) for --sf-size-m; when CSS resolves var(--sf-size-m) for that element, var(--sf-density) is looked up in that element's context → 0.8, giving 2rem. The behaviour the PR says was "verified" (no rescale) only holds when reading the size from the parent element after setting density on a child — the child and its descendants do see the rescaled value. Documenting this as strictly :root-only misinforms developers who would benefit from component-level density control, which actually works by design.
There was a problem hiding this comment.
I checked this carefully before pushing the "does not rescale" claim, and re-verified it again just now — the documented behavior is correct, and this comment describes a plausible-sounding but incorrect mental model of custom-property substitution.
The key detail: var() substitution inside a custom property's own value happens at the element where that custom property is declared (or wins the cascade), not lazily re-evaluated per consuming descendant. --sf-size-m: calc(2.5rem * var(--sf-density)) is declared once, at :root. Chromium resolves var(--sf-density) there (→ 1) and the already-substituted token stream (calc(2.5rem * 1)) is what inherits down — not the original var(--sf-density) reference. A descendant overriding --sf-density does change its own getComputedStyle(...).getPropertyValue('--sf-density'), but --sf-size-m on that descendant still reads calc(2.5rem * 1), because it was never re-declared in that scope.
Verified two ways just now:
- Minimal isolated case (no SLASHED CSS at all):
:root { --k: 1; --derived: calc(10px * var(--k)); } .child { --k: 5; }
<div class="child" style="width:var(--derived)">→ computed width is 10px, not 50px. - Same result reproduced against the actual built
dist/slashed.full.csswith--sf-densityon a scoped<aside>.
This isn't a novel claim either — it's the exact same phenomenon core/layout.css already documents for --sf-fluid-width (see the .sf-fluid-cq comment): "the --sf-text-* / --sf-space-* tokens are computed at :root, so their var(--sf-fluid-width) is substituted there... overriding --sf-fluid-width lower in the tree can't retroactively rewrite an already-computed inherited value." --sf-density has the identical resolution shape, so the identical caveat applies.
If someone genuinely wants scoped density, the fix mirrors .sf-fluid-cq: re-declare the --sf-size-* ladder (not just --sf-density) on the scoping element's children. That's a real, addressable feature gap, but documenting today's behavior as "already works by design" would be inaccurate and would mislead people who try <aside style="--sf-density: 0.8"> expecting it to work. Keeping the current wording.
Generated by Claude Code
The --sf-density commit added a row for the new token but left the existing --sf-size-xs..xl rows stale (role: knob, literal rem values) — they now derive through --sf-density (role: consumption, calc() expressions), matching docs/token-index.json. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TsAowhRHNTVVfxvafVtDas
2 participants
Summary
Adds
--sf-density(PUBLIC-ADVANCED, default1) — the compact ↔ comfortable dial for interactive control geometry. It scales the--sf-size-*rung ladder (24·32·40·48·56px at1): below1packs controls tighter (dashboards, data tables, power-user tools), above1loosens them.Motivation: the size ladder was previously static rem literals scaled by no knob at all — intentionally non-fluid, because control height must not shrink on small screens where touch targets need to grow (unlike
--sf-text-*/--sf-space-*, which are viewport-fluid by design). That left a real gap: a project wanting denser UI had to hand-override every--sf-size-*rung plus related paddings and hope they stayed in sync.--sf-densitygives that whole geometric axis one deliberate dial while preserving the non-fluid contract — it's a product/user-preference choice, never an automatic viewport response.--sf-densityis orthogonal to--sf-space-scale(overall whitespace) and--sf-section-scale(section rhythm); combine them for a fully compact UI. The--sf-touch-targetaccessibility floor stays independent, so a small value can't pull native controls under the WCAG target. Like the other global multipliers, it's a:rootdial — the size ladder is computed at:rootand inherits, so a nested override does not retroactively rescale it (same computed-value rule as--sf-space-scaleet al.; documented explicitly to avoid a false claim about subtree scoping).Changes
Tokens
--sf-density(knob,1) — global multiplier for the--sf-size-*ladderCSS
core/tokens.css: new--sf-densitytoken;--sf-size-xs…xlre-pointed throughcalc(<literal> * var(--sf-density))Tooling / wiring
scripts/token-tiers.js:--sf-densityclassified PUBLIC-ADVANCED (alongside the other global multipliers)docs/token-annotations.json: annotation addeddocs/llm-guide.md: added to the §10 Global Scale Multipliers table with usage notesconfigurator/src/data/domain-patterns.json:densitypattern added to thespacingdomain (keeps the curation test passing)demos/generate.mjs: curated override value for the "ultimate override" demodocs/api-index.*,docs/token-index.*,docs/tokens.md,docs/classes.md,docs/registry.json,token-registry.json,configurator/src/data/*.generated.json,dist/css-custom-data.json, static coverage artifacts (docs/test-coverage-6-token-reference.html,reports/full-api-audit/results/tokens-report.json)Tests
tests/tokens.spec.js: new test verifying--sf-densityuniformly scales the--sf-size-*ladder (0.8× and 1.25×) against the default 24/32/40/48/56px rungsBug fix (found while implementing this)
core/tokens.css: a comment near the new token contained--sf-text-*/--sf-space-*— the*/prematurely closed the CSS comment, which would have brokentests/tier1-p2-coverage.test.js(the same class of bug fixed incore/motion.cssin the prior.sf-staggerPR). Reworded to avoid the sequence; audited the entirecore/,optional/,demos/,configurator/srcCSS tree for the same pattern (character-level parser + two complementary greps) — no other occurrences found.Type
Checklist
npm run lint:csspassesnpm run buildrebuildsdist/npm run test:unitpasses (111/111); Playwright e2e verified manually against the installed browser in this environment (full multi-browser e2e runs on CI)npm run check:versionpasses (no version bump needed)npm run check:llm-guidepasses (LLM guide reviewed and updated)npm run docs,npm run gen:registry,npm run audit), not hand-editedCHANGELOG.mdupdated under## [Unreleased]--sf-densityis additive; the--sf-size-*ladder's default resolved values are unchanged at--sf-density: 1Notes
Manually verified in a real Chromium instance:
--sf-size-mresolves to 40px at--sf-density: 1, 32px at0.8, 50px at1.25;.sf-btn's default min-height tracks the ladder. Also verified that--sf-densityset on a nested element does not rescale an already-computed inherited size token — confirming it must be documented as a:root-only dial, consistent with the framework's other scale multipliers.https://claude.ai/code/session_01TsAowhRHNTVVfxvafVtDas
Generated by Claude Code
Summary by CodeRabbit
New Features
--sf-densitycontrol to switch interactive sizing between compact and comfortable modes.--sf-densityscales the--sf-size-*rung ladder while keeping the touch-target accessibility floor independent.--sf-density.Documentation
Tests
--sf-density-driven scaling behavior.