Fix layout utilities and dark mode color tokens - #15
Conversation
…on, swatches - Add missing .sf-cluster--m modifier to layout.css - Add base class to all modifier-only sf-cluster/sf-stack usages in demo.html - Fix @Keyframes demo-slide to use left instead of translate (track-relative movement) - Add left: 0 to .demo-ease-ball for correct animation start position - Add dark mode text/heading token overrides to both dark mode blocks in base.css (Bug 4) - Add dark mode border token overrides with correct luminance direction (Bug 5) - Increase --sf-shadow-strength to 0.25 in dark mode for visible shadows (Bug 6) - Enlarge .demo-radius-swatch to 6rem×5rem to distinguish small radii (Bug 7) - Change Border Radius section container align-items from flex-end to center https://claude.ai/code/session_018HDAUATPwLueARYL8EtLRS
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExtends dark-mode semantic CSS variables and tightens token math in ChangesCore theme and demo updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/demo.html (1)
236-236: ⚡ Quick winPrefer
transform: translateX()overleftfor animation performance.The animation now uses
leftpositioning, which triggers layout recalculation on every frame.transform: translateX()is GPU-accelerated and significantly more performant, especially with multiple concurrent animations (6 easing balls). Since this demo showcases best practices for the design system, consider reverting to transform-based animation.⚡ Proposed fix using transform
.demo-ease-ball { width: 2rem; height: 2rem; border-radius: var(--sf-radius-full); background: var(--sf-color-primary); position: absolute; top: 0; - left: 0; + left: 0; /* Keep for initial positioning */ } `@keyframes` demo-slide { - from { left: 0; } - to { left: calc(100% - 2rem); } + from { transform: translateX(0); } + to { transform: translateX(calc(100% - 2rem)); } }Also applies to: 240-241
🤖 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 `@docs/demo.html` at line 236, The CSS uses left positioning for the animation (e.g., the rule containing "left: 0;") which causes layout thrashing; replace the left-based animation with transform-based translation: update the relevant keyframes and any rules that set or animate left to instead use transform: translateX(...) or translate3d(...,0,0) for GPU acceleration, and if JavaScript manipulates element.style.left (or any code referencing left at the demo around the same block), change it to update a transform (or a CSS custom property used in transform) so the animation uses translateX and not left; also adjust any positioning logic so the visual layout and offsets match after switching (apply the same change to the other occurrences noted at the nearby lines).
🤖 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.
Nitpick comments:
In `@docs/demo.html`:
- Line 236: The CSS uses left positioning for the animation (e.g., the rule
containing "left: 0;") which causes layout thrashing; replace the left-based
animation with transform-based translation: update the relevant keyframes and
any rules that set or animate left to instead use transform: translateX(...) or
translate3d(...,0,0) for GPU acceleration, and if JavaScript manipulates
element.style.left (or any code referencing left at the demo around the same
block), change it to update a transform (or a CSS custom property used in
transform) so the animation uses translateX and not left; also adjust any
positioning logic so the visual layout and offsets match after switching (apply
the same change to the other occurrences noted at the nearby lines).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c35040e-ab96-42d2-8db9-0e133653ce10
📒 Files selected for processing (3)
core/base.csscore/layout.cssdocs/demo.html
jackgranatowski
left a comment
jackgranatowski
left a comment
There was a problem hiding this comment.
Skipping the transform: translateX() suggestion — the proposed fix reintroduces the exact bug this PR fixed.
In transform: translateX(), 100% resolves to the element's own width (2rem), so calc(100% - 2rem) = calc(2rem - 2rem) = 0 — the ball doesn't move. This was the original bug.
Using left, 100% resolves to the containing block's width (the track), which is what we need. The performance trade-off is negligible for a 6-ball demo page.
Generated by Claude Code
fix(base): dark mode status text contrast — lightened with calc(l + 0.15)
The #main { flex: 1 } rule was overriding .sf-sidebar > :last-child { flex-grow: 999 },
causing nav and main to share space equally (~50/50) instead of 1:999.
Removing flex:1 restores correct framework behavior. Sidebar width reduced
from 14rem to 12rem — sufficient for all nav labels.
Status text tokens (success/warning/error/info/danger) used calc(l - X) which
in dark mode produced l≈0.50 text on l≈0.22 background. Overrides in both
dark mode blocks flip to calc(l + X) giving l≈0.80 — clearly readable.
https://claude.ai/code/session_018HDAUATPwLueARYL8EtLRS
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/base.css (2)
92-117: ⚖️ Poor tradeoffConsider reducing duplication between dark mode blocks.
The derived color declarations (text, borders, shadow, status) in
[data-theme="dark"](lines 92-117) are identical to those in the OS preference block (lines 39-63). This duplication creates maintenance burden and risk of inconsistency.While CSS doesn't easily support sharing these declarations across different selectors, consider:
- Using a CSS preprocessor mixin
- Combining selectors if possible:
:where([data-theme="dark"], :root:not([data-theme])):where(...)- Documenting that both blocks must be kept in sync
🤖 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 `@core/base.css` around lines 92 - 117, The dark-mode block under [data-theme="dark"] duplicates many derived CSS variable declarations (e.g., --sf-color-text, --sf-color-border, --sf-shadow-strength, --sf-status-success-text, etc.) that already exist in the OS preference block; refactor by extracting these shared variable declarations into a single rule applied to both contexts (for example combine selectors or use :where([data-theme="dark"], :root:where(...)) to target both dark contexts) or move them into a reusable preprocessor mixin and include it in both places so the declarations are maintained in one place and the duplicated blocks are removed/updated accordingly.
39-46: ⚡ Quick winAdd fallback colors and consider reducing duplication between OS preference and explicit theme blocks.
The
oklch(from ...)relative color syntax has strong browser support (Chrome 119+, Firefox 128+, Safari 16.4+). However, for graceful degradation in older browsers and as a best practice, provide fallback declarations before the relative color rules. Additionally, the identical color declarations across lines 39–46 (and 47–63) are duplicated in the[data-theme="dark"]block at lines 93–99 (and 101–116); consider extracting these to a shared rule or using a preprocessor to maintain a single source of truth.Example fallback pattern
--sf-color-text: `#ffffff`; /* Fallback for unsupported browsers */ --sf-color-text: oklch(from var(--sf-color-neutral) calc(l + 0.25) c h);🤖 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 `@core/base.css` around lines 39 - 46, Add fallback color declarations before each oklch(...) variable assignment for graceful degradation (e.g., set --sf-color-text, --sf-color-text--secondary, --sf-color-text--placeholder, --sf-color-text--disabled, --sf-color-text--inverse, --sf-color-heading to solid hex values before their oklch(...) lines), and remove duplicated assignments by extracting the shared variables into a single shared rule (e.g., :root or a .theme-common block) used by both the prefers-color-scheme: dark and [data-theme="dark"] blocks so the same variable names (--sf-color-text, --sf-color-text--secondary, --sf-color-text--placeholder, --sf-color-text--disabled, --sf-color-text--inverse, --sf-color-heading) are defined once and then overridden only where necessary.
🤖 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.
Nitpick comments:
In `@core/base.css`:
- Around line 92-117: The dark-mode block under [data-theme="dark"] duplicates
many derived CSS variable declarations (e.g., --sf-color-text,
--sf-color-border, --sf-shadow-strength, --sf-status-success-text, etc.) that
already exist in the OS preference block; refactor by extracting these shared
variable declarations into a single rule applied to both contexts (for example
combine selectors or use :where([data-theme="dark"], :root:where(...)) to target
both dark contexts) or move them into a reusable preprocessor mixin and include
it in both places so the declarations are maintained in one place and the
duplicated blocks are removed/updated accordingly.
- Around line 39-46: Add fallback color declarations before each oklch(...)
variable assignment for graceful degradation (e.g., set --sf-color-text,
--sf-color-text--secondary, --sf-color-text--placeholder,
--sf-color-text--disabled, --sf-color-text--inverse, --sf-color-heading to solid
hex values before their oklch(...) lines), and remove duplicated assignments by
extracting the shared variables into a single shared rule (e.g., :root or a
.theme-common block) used by both the prefers-color-scheme: dark and
[data-theme="dark"] blocks so the same variable names (--sf-color-text,
--sf-color-text--secondary, --sf-color-text--placeholder,
--sf-color-text--disabled, --sf-color-text--inverse, --sf-color-heading) are
defined once and then overridden only where necessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e81f4e2c-7a27-455d-a3d7-e8803bea6708
📒 Files selected for processing (2)
core/base.cssdocs/demo.html
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/demo.html
jackgranatowski
left a comment
jackgranatowski
left a comment
There was a problem hiding this comment.
Skipping both nitpicks.
Duplication between @media and [data-theme="dark"] blocks: acknowledged but not fixable in pure CSS without a build tool. There is no CSS selector syntax that can simultaneously be conditional on a media feature and an attribute — the two blocks serve structurally distinct cascade roles and cannot be merged. A Sass mixin would solve it, but this framework has no build step. Added an in-code comment noting both blocks must be kept in sync.
oklch() fallbacks: the entire framework already depends on oklch() relative color syntax throughout tokens.css (brand colors, surface tokens, border tokens, shadows — everything). Adding fallbacks only to the dark mode overrides in base.css would be inconsistent and ineffective — a browser without support for relative oklch() colors would already fail at tokens.css long before reaching these rules. Modern browser support (Chrome 119+, Firefox 128+, Safari 16.4+) is a documented framework requirement.
Generated by Claude Code
CSS cannot share declarations across a media condition and a selector, so the derived semantic overrides are intentionally duplicated. https://claude.ai/code/session_018HDAUATPwLueARYL8EtLRS
…om palettes --sf-color-text--on-*: replaced hardcoded white/dark with sign(0.6 - l) * 999 inside clamp(0.1, ..., 0.95). Works for any source color luminance and adapts automatically in dark mode (tokens reference the active -dark color variant). Example: yellow primary (l=0.85) → dark text; red primary (l=0.45) → light text. Added clamp() guards to all directional light-mode formulas in tokens.css (text, heading, border) and to dark-mode overrides in base.css (both blocks). Prevents broken output when a developer uses neutral values outside the range the offset formulas were tuned for, with no change to computed values for the default palette. https://claude.ai/code/session_018HDAUATPwLueARYL8EtLRS
2 participants
Summary
This PR fixes missing layout utility classes, improves dark mode color token definitions, and corrects animation implementation in the demo page.
Key Changes
Layout Utilities (core/layout.css)
.sf-cluster--mgap modifier class that was referenced throughout the demo but not defined in the CSSDark Mode Color Tokens (core/base.css)
@media (prefers-color-scheme: dark)and.darkclass selectoroklch()relative color syntax with adjusted lightness values for dark backgrounds--sf-shadow-strength: 0.25) for better visibility on dark surfacesDemo Page Fixes (docs/demo.html)
4.5rem × 4.5remto6rem × 5remfor better visibilitytranslateproperty toleftproperty with explicitleft: 0positioning for more reliable animation behavior.sf-clusterto all.sf-cluster--*elements.sf-stackto all.sf-stack--*elementsalign-items: flex-endtoalign-items: centerfor better visual balanceImplementation Details
oklch(from ...)) to derive values from the neutral color variable, ensuring consistency+0.25for primary text,+0.1for secondary,-0.1for placeholder,-0.2for disabled,-0.4for inverse0.005for subtle/normal,0.02for strong) to maintain proper contrast on dark backgroundshttps://claude.ai/code/session_018HDAUATPwLueARYL8EtLRS
Summary by CodeRabbit
Style
Documentation