refactor(api): rename source tokens -light/-dark → -source-light/-source-dark, merge error into danger, add sf-section--guttered - #387
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 27 minutes and 16 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRenames all color source knob tokens from ChangesSLASHED 0.7.0 Color System Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2eab656fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @property --sf-color-tertiary-light { syntax: "<color>"; inherits: true; initial-value: oklch(0.42 0.22 295); } | ||
| @property --sf-color-action-light { syntax: "<color>"; inherits: true; initial-value: oklch(0.50 0.22 235); } | ||
| @property --sf-color-neutral-light { syntax: "<color>"; inherits: true; initial-value: oklch(0.52 0.025 260); } | ||
| @property --sf-color-primary-source-light { syntax: "<color>"; inherits: true; initial-value: oklch(0.47 0.27 264); } |
There was a problem hiding this comment.
After this source-token rename, the configurator's BrandColorRow.svelte still builds --sf-color-${colorKey}-light / --sf-color-${colorKey}-dark dynamically and setOverride accepts those names, so editing any color in the main Colors panel now exports dead custom properties instead of changing --sf-color-*-source-light/source-dark. Update that editor (and its key list) along with the token rename so the live preview and generated CSS actually affect the new source tokens.
Useful? React with 👍 / 👎.
| --sf-color-success: var(--sf-color-success-source-light); | ||
| --sf-color-warning: var(--sf-color-warning-source-light); | ||
| --sf-color-info: var(--sf-color-info-source-light); | ||
| --sf-color-danger: var(--sf-color-danger-source-light); |
There was a problem hiding this comment.
Only the danger token is published here after removing the --sf-color-error* declarations. In projects that already use var(--sf-color-error), --sf-color-error-subtle/muted/strong, or --sf-color-text--on-error, those references now resolve as missing instead of mapping to danger, despite the merge being described as an alias in the code/comments. Keep the old error custom properties as aliases to the corresponding danger tokens to avoid breaking existing consumers.
Useful? React with 👍 / 👎.
jackgranatowski
force-pushed
the
claude/design-system-api-feedback-xmomho
branch
2 times, most recently
from
June 23, 2026 21:12
b182c92 to
88a7817
Compare
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
configurator/src/data/api-index.generated.json (1)
7076-7077: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the legacy
erroralias in the published index.Line 7076 removes
--sf-color-text--on-error, but the PR objective says theerrorfamily should remain as backwards-compatible aliases. Publishing only thedangernames breaks consumers that still resolve the old API surface.Based on the PR objective: the
errorfamily should remain as backwards-compatible aliases.🤖 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 `@configurator/src/data/api-index.generated.json` around lines 7076 - 7077, The change is removing the `--sf-color-text--on-error` entry from the api-index.generated.json file, which breaks backwards compatibility for consumers still using the legacy error naming convention. According to the PR objective, the error family should remain as backwards-compatible aliases alongside the new danger naming. Restore the `--sf-color-text--on-error` entry and all other legacy error family aliases in the published index to maintain the complete API surface for existing consumers.core/macros.css (1)
221-226: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the named surface variant count.
After removing
.sf-surface--error, this section lists 10 precomputed variants, not 11.📝 Proposed fix
- contextual surface tokens. 11 precomputed variants + contextual surface tokens. 10 precomputed variants🤖 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/macros.css` around lines 221 - 226, The documentation comment for the Surface macro incorrectly states there are "11 precomputed variants" when there are actually only 10 after the removal of `.sf-surface--error`. Update the number in the comment block that begins with "/* -- Surface" to reflect the correct count of 10 precomputed variants.
🤖 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 `@core/themes.css`:
- Around line 99-112: Update all comments associated with the color variable
declarations to reflect the new naming convention. In the CSS theme file, find
the comments near the declarations for --sf-color-primary, --sf-color-secondary,
--sf-color-tertiary, and --sf-color-action (around lines 99-112 and 180-184) and
replace references to the old naming pattern (such as `-light`, `-dark`, and
`--sf-color-X-dark`) with references to the new `--sf-color-*-source-light` and
`--sf-color-*-source-dark` format to ensure consumers have accurate guidance on
which variables to override.
In `@core/tokens.css`:
- Around line 10-12: The comments at lines 10-12 describe the source-dark tokens
as unregistered and optional, but the actual `@property` registrations for
--sf-color-*-source-dark tokens are defined at lines 211-220, making the
comments outdated and misleading. Update the comments at lines 10-12 and also at
lines 161-163 to accurately reflect that these source-dark tokens are now
registered with `@property`, removing any language that suggests they are
unregistered or purely optional overrides. Ensure the comments align with the
actual token registrations present in the file.
- Around line 631-634: The `--sf-color-selection-bg` variable definition in the
dark mode branch (the second argument in the light-dark() function) is
incorrectly using `var(--sf-color-action-source-light)` instead of
`var(--sf-color-action-source-dark)`. Change the color source in the dark
selection oklch() call from `--sf-color-action-source-light` to
`--sf-color-action-source-dark` so that dark mode overrides of the action source
token are properly respected. Also ensure that any section-level redeclaration
of this variable in core/themes.css is updated to match this change.
In `@docs/architecture.md`:
- Line 205: The source color count listed in the property registration
description is incorrect. The text states "18 source colours" but the breakdown
provided (10 -source-light + 10 -source-dark) totals 20, not 18. Update the
count from 18 to 20 to accurately reflect the actual number of source colors in
the 0.7.0 source-token set.
In `@docs/demo.html`:
- Line 2778: The HEX_FALLBACKS object uses outdated key naming with the
`--sf-color-*-light` pattern, but the picker inputs now expect
`--sf-color-*-source-light` and `--sf-color-*-source-dark` data-tokens. Update
all fallback keys in HEX_FALLBACKS by replacing the legacy `-light` suffix with
`-source-light` and add corresponding `-source-dark` entries for each of the 20
color tokens to match the new naming convention used by the picker inputs.
- Around line 803-808: In the sf-cluster div containing the status tiles, there
are two duplicate "Danger" tiles. Remove the second Danger tile that appears
after the Info tile (the div with text "Danger" that uses sf-color-danger-subtle
variables), as it creates a redundant status in the cluster. If a fifth status
indicator is needed, consider replacing it with an appropriate alternative
status type instead of duplicating Danger.
In `@docs/test-coverage-1-colors.html`:
- Around line 293-299: The fourth demo-chip div in the section uses the correct
CSS variables (--sf-color-danger and --sf-color-text--on-danger) but displays
the outdated label text "on-error" instead of "on-danger". Change the visible
text content of this demo-chip from "on-error" to "on-danger" to align with the
tokens being used and match the sixth demo-chip which correctly displays
"on-danger".
In `@docs/test-coverage-4-macros-states.html`:
- Line 105: The demo cell in the HTML file has the CSS class sf-surface--danger
applied to it, but the text content displayed inside the div still reads
"surface--error". Update the text content of this div element to say
"surface--danger" instead, so the label matches the CSS class being
demonstrated.
In `@docs/test-coverage-6-token-reference.html`:
- Line 113: The TOKENS array contains duplicate token name entries which causes
inaccurate counts and duplicate rows in the output. Remove all duplicate entries
from the TOKENS array variable to ensure it contains only unique token names.
You can verify the fix by checking that each token name appears exactly once in
the array.
---
Outside diff comments:
In `@configurator/src/data/api-index.generated.json`:
- Around line 7076-7077: The change is removing the `--sf-color-text--on-error`
entry from the api-index.generated.json file, which breaks backwards
compatibility for consumers still using the legacy error naming convention.
According to the PR objective, the error family should remain as
backwards-compatible aliases alongside the new danger naming. Restore the
`--sf-color-text--on-error` entry and all other legacy error family aliases in
the published index to maintain the complete API surface for existing consumers.
In `@core/macros.css`:
- Around line 221-226: The documentation comment for the Surface macro
incorrectly states there are "11 precomputed variants" when there are actually
only 10 after the removal of `.sf-surface--error`. Update the number in the
comment block that begins with "/* -- Surface" to reflect the correct count of
10 precomputed variants.
🪄 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: f10748ab-fe29-40bb-89b6-d1fe2cc8e688
⛔ Files ignored due to path filters (2)
dist/badge-essential.jsonis excluded by!**/dist/**dist/badge-optimal.jsonis excluded by!**/dist/**
📒 Files selected for processing (56)
README.mdconfigurator/src/components/BrandColorRow.svelteconfigurator/src/components/DomainPanel.svelteconfigurator/src/components/Preview.svelteconfigurator/src/data/api-index.generated.jsonconfigurator/src/data/token-registry.generated.jsonconfigurator/src/lib/brandColors.jsconfigurator/src/lib/themes.jsconfigurator/src/styles/app.cssconfigurator/tests-e2e/import-export.spec.jsconfigurator/tests-e2e/preview.spec.jsconfigurator/tests/smoke.test.jsconfigurator/tests/store-history.test.jsconfigurator/tests/themes.test.jscore/layout.csscore/macros.csscore/motion.csscore/states.csscore/themes.csscore/tokens.cssdocs/api-index.jsondocs/api-index.mddocs/architecture.mddocs/classes.mddocs/demo.htmldocs/llm-guide.mddocs/migration.mddocs/registry.jsondocs/test-coverage-1-colors.htmldocs/test-coverage-2-typography.htmldocs/test-coverage-3-layout.htmldocs/test-coverage-4-macros-states.htmldocs/test-coverage-5-forms-features.htmldocs/test-coverage-6-token-reference.htmldocs/theming.mddocs/token-annotations.jsondocs/token-index.jsondocs/token-index.mddocs/tokens.mdoptional/components.cssoptional/config-example.cssoptional/forms.cssoptional/theme-example.cssoptional/tokens.palette.cssscripts/check-token-registry.jstests/auto-color.spec.jstests/baseline-modern.jsontests/color-semantic.spec.jstests/demo-a11y-panel.spec.jstests/demo-visual.spec.jstests/link-contrast.spec.jstests/tier1-p7-oldengine.test.jstests/tier1-p8-modern.spec.jstests/token-api.snapshot.jsontests/tokens.spec.jstoken-registry.json
💤 Files with no reviewable changes (1)
- tests/baseline-modern.json
… add sf-section--guttered - Rename all 20 --sf-color-X-light/-dark source input tokens to --sf-color-X-source-light/-source-dark (10 brand + 10 status families) to eliminate naming collision with shade-alias suffixes (-lighter, -xlight, etc.) - Remove --sf-color-error* family entirely (replaced by --sf-color-danger*) - Update .is-invalid/.is-error states, .sf-badge--*, .sf-alert--* to use danger tokens - Add .sf-section--guttered with double-gutter prevention for nested containers - Update configurator BrandColorRow/DomainPanel/brandColors to use new token names - Remove --sf-color-error from BrandColorRow colour list - Relax check-token-registry.js invariant 1: allow intentional in-place renames Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015eRstwpktwGixBeiKTKhkk
- Remove 'error' from status arrays in auto-color, color-semantic, tokens specs - demo-a11y-panel: 13→12 pairs, swap 'Text on error' for 'Text on danger' - demo-visual: fix count 5→4 status swatches, fix bgs.size 5→4 - demo.html: remove duplicate danger swatches (color row + surfaces grid) - demo.html: add sf-section--guttered demo for coverage test - demo.html: update status text copy (error → danger/warning)
Update badge sizes and api-index hash to reflect current build state post-rebase onto main (v0.6.11).
… configurator - Rename --sf-color-*-light → --sf-color-*-source-light in all test-coverage :root blocks (files 1-6) - Fix demo.html code example snippet and animation description text to reference --sf-color-primary-source-light (motion.css already animates the source-light token) - Fix Preview.svelte code example snippet - Fix test-coverage-6 Array.from(new Set([..."])) closing parens - Fix test-coverage-4/1 surface--danger/on-danger label text - Update tests/token-api.snapshot.json to post-rename sort order - Fix core/tokens.css: dark branch of --sf-color-selection-bg uses --sf-color-action-source-dark; fix stale comments - Fix core/themes.css stale -light/-dark comments - Fix core/macros.css: 11→10 precomputed variants comment - Fix docs/architecture.md: 18→20 source colours
…facts Document all 10 --sf-color-*-source-dark tokens in sections 5.2 and 5.3 of docs/llm-guide.md. Regenerate api-index, token-index, and tokens.md after build.
jackgranatowski
force-pushed
the
claude/design-system-api-feedback-xmomho
branch
from
June 24, 2026 04:19
88a7817 to
5dfbc6c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/test-coverage-1-colors.html (1)
237-246: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove duplicated
dangerentries from the status demo rows.Line 237 and Line 246 were migrated from error→danger, but each row already contains another danger item, so status coverage now duplicates one status and drops variety.
Proposed fix
<div class="demo-swatch-row"> <div class="demo-swatch" style="background: var(--sf-color-success)">success</div> <div class="demo-swatch" style="background: var(--sf-color-warning)">warning</div> <div class="demo-swatch" style="background: var(--sf-color-danger)">danger</div> <div class="demo-swatch" style="background: var(--sf-color-info)">info</div> - <div class="demo-swatch" style="background: var(--sf-color-danger)">danger</div> </div> @@ <div class="sf-cluster sf-cluster--m"> <div class="demo-chip" style="background: var(--sf-color-success-subtle); color: var(--sf-color-success-strong); border: 1px solid var(--sf-color-success-muted)">Success</div> <div class="demo-chip" style="background: var(--sf-color-warning-subtle); color: var(--sf-color-warning-strong); border: 1px solid var(--sf-color-warning-muted)">Warning</div> <div class="demo-chip" style="background: var(--sf-color-danger-subtle); color: var(--sf-color-danger-strong); border: 1px solid var(--sf-color-danger-muted)">Danger</div> <div class="demo-chip" style="background: var(--sf-color-info-subtle); color: var(--sf-color-info-strong); border: 1px solid var(--sf-color-info-muted)">Info</div> - <div class="demo-chip" style="background: var(--sf-color-danger-subtle); color: var(--sf-color-danger-strong); border: 1px solid var(--sf-color-danger-muted)">Danger</div> </div>🤖 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/test-coverage-1-colors.html` around lines 237 - 246, The demo-swatch row contains duplicate danger status entries on lines 237 and 239, reducing coverage variety. Replace one of the duplicate danger demo-swatch divs with a different status color variable (such as success, warning, or info) that is not already represented in that row to maintain consistent status coverage across all demo sections. Ensure the replacement div maintains the same structure as the other demo-swatch elements with the appropriate CSS custom property for the chosen status.
🧹 Nitpick comments (1)
configurator/src/components/Preview.svelte (1)
1235-1235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove duplicate
.pv__surface-dangerselector to prevent drift.Line 1235 defines
.pv__surface-danger, but the same selector is defined again at Line 1237 with identical declarations. Keep a single source of truth.Suggested cleanup
.pv__surface-warning { background: var(--sf-color-warning-subtle, rgba(202,138,4,.1)); color: var(--sf-color-warning-strong,`#854d0e`); } .pv__surface-danger { background: var(--sf-color-danger-subtle, rgba(220,38,38,.1)); color: var(--sf-color-danger,`#dc2626`); } .pv__surface-info { background: var(--sf-color-info-subtle, rgba(8,145,178,.1)); color: var(--sf-color-info,`#0891b2`); } - .pv__surface-danger { background: var(--sf-color-danger-subtle, rgba(220,38,38,.1)); color: var(--sf-color-danger,`#dc2626`); }🤖 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 `@configurator/src/components/Preview.svelte` at line 1235, The `.pv__surface-danger` CSS selector is defined twice with identical declarations in the stylesheet. Locate both definitions of the `.pv__surface-danger` selector and remove one of the duplicate declarations, keeping only a single definition to maintain a single source of truth and prevent unnecessary CSS drift.
🤖 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 `@configurator/src/data/api-index.generated.json`:
- Around line 3398-3406: The `--sf-color-heading` token's note claims it has
"slightly stronger contrast than body text for visual hierarchy," but the
current value formula is identical to `--sf-color-text`, making the semantic
contract misleading. Either adjust the oklch() formula values in both the
light-dark branches (the clamp constraints or the arithmetic operations on l, c,
h values) to make the heading color produce noticeably stronger contrast
compared to body text, or update the note to accurately reflect that they are
semantically equivalent. Check all other occurrences of this token around line
numbers 6910-6918 to apply the same fix consistently across the file.
- Around line 1467-1470: The group label "DARK SOURCE TOKENS (-dark,
animatable)" in the api-index.generated.json file is outdated and exposes the
deprecated suffix. Since this is a generated artifact, do not edit it directly.
Instead, locate the source metadata configuration that generates this file,
update all occurrences of the group label to reflect the new naming convention
and remove the reference to the old (-dark, animatable) suffix, then regenerate
the api-index.generated.json file to ensure all affected entries (including
those at the line ranges specified in the comment) are updated consistently.
---
Outside diff comments:
In `@docs/test-coverage-1-colors.html`:
- Around line 237-246: The demo-swatch row contains duplicate danger status
entries on lines 237 and 239, reducing coverage variety. Replace one of the
duplicate danger demo-swatch divs with a different status color variable (such
as success, warning, or info) that is not already represented in that row to
maintain consistent status coverage across all demo sections. Ensure the
replacement div maintains the same structure as the other demo-swatch elements
with the appropriate CSS custom property for the chosen status.
---
Nitpick comments:
In `@configurator/src/components/Preview.svelte`:
- Line 1235: The `.pv__surface-danger` CSS selector is defined twice with
identical declarations in the stylesheet. Locate both definitions of the
`.pv__surface-danger` selector and remove one of the duplicate declarations,
keeping only a single definition to maintain a single source of truth and
prevent unnecessary CSS drift.
🪄 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: fc1e3a7d-9fc5-4c21-8822-4f9e95a8e6fa
⛔ Files ignored due to path filters (2)
dist/badge-essential.jsonis excluded by!**/dist/**dist/badge-optimal.jsonis excluded by!**/dist/**
📒 Files selected for processing (56)
README.mdconfigurator/src/components/BrandColorRow.svelteconfigurator/src/components/DomainPanel.svelteconfigurator/src/components/Preview.svelteconfigurator/src/data/api-index.generated.jsonconfigurator/src/data/token-registry.generated.jsonconfigurator/src/lib/brandColors.jsconfigurator/src/lib/themes.jsconfigurator/src/styles/app.cssconfigurator/tests-e2e/import-export.spec.jsconfigurator/tests-e2e/preview.spec.jsconfigurator/tests/smoke.test.jsconfigurator/tests/store-history.test.jsconfigurator/tests/themes.test.jscore/layout.csscore/macros.csscore/motion.csscore/states.csscore/themes.csscore/tokens.cssdocs/api-index.jsondocs/api-index.mddocs/architecture.mddocs/classes.mddocs/demo.htmldocs/llm-guide.mddocs/migration.mddocs/registry.jsondocs/test-coverage-1-colors.htmldocs/test-coverage-2-typography.htmldocs/test-coverage-3-layout.htmldocs/test-coverage-4-macros-states.htmldocs/test-coverage-5-forms-features.htmldocs/test-coverage-6-token-reference.htmldocs/theming.mddocs/token-annotations.jsondocs/token-index.jsondocs/token-index.mddocs/tokens.mdoptional/components.cssoptional/config-example.cssoptional/forms.cssoptional/theme-example.cssoptional/tokens.palette.cssscripts/check-token-registry.jstests/auto-color.spec.jstests/baseline-modern.jsontests/color-semantic.spec.jstests/demo-a11y-panel.spec.jstests/demo-visual.spec.jstests/link-contrast.spec.jstests/tier1-p7-oldengine.test.jstests/tier1-p8-modern.spec.jstests/token-api.snapshot.jsontests/tokens.spec.jstoken-registry.json
💤 Files with no reviewable changes (1)
- tests/baseline-modern.json
✅ Files skipped from review due to trivial changes (12)
- tests/tokens.spec.js
- tests/demo-a11y-panel.spec.js
- tests/tier1-p7-oldengine.test.js
- docs/classes.md
- README.md
- docs/architecture.md
- optional/tokens.palette.css
- configurator/tests/smoke.test.js
- configurator/tests/store-history.test.js
- docs/theming.md
- tests/token-api.snapshot.json
- docs/api-index.json
🚧 Files skipped from review as they are similar to previous changes (34)
- configurator/tests-e2e/preview.spec.js
- tests/tier1-p8-modern.spec.js
- core/motion.css
- configurator/src/lib/themes.js
- configurator/src/components/DomainPanel.svelte
- configurator/src/components/BrandColorRow.svelte
- optional/config-example.css
- core/layout.css
- optional/components.css
- tests/demo-visual.spec.js
- tests/auto-color.spec.js
- configurator/src/styles/app.css
- tests/link-contrast.spec.js
- core/states.css
- optional/forms.css
- docs/test-coverage-6-token-reference.html
- scripts/check-token-registry.js
- configurator/src/lib/brandColors.js
- configurator/tests/themes.test.js
- configurator/tests-e2e/import-export.spec.js
- core/macros.css
- tests/color-semantic.spec.js
- optional/theme-example.css
- docs/token-index.md
- token-registry.json
- configurator/src/data/token-registry.generated.json
- docs/migration.md
- docs/llm-guide.md
- docs/tokens.md
- docs/api-index.md
- docs/token-annotations.json
- core/themes.css
- core/tokens.css
- docs/token-index.json
Comment on lines
1467
to
+1470
| "group": "DARK SOURCE TOKENS (-dark, animatable)", | ||
| "description": "OKLCH dark source tokens for brand and status colors. Registered as typed <color> so scoped [data-theme=\"dark\"] sections inherit correct absolute values rather than an unresolved var() text string.", | ||
| "note": "Primary interactive color. Applied to filled buttons, active indicators, and brand accent surfaces.", | ||
| "value": "light-dark(var(--sf-color-action-light), var(--sf-color-action-dark, oklch(from var(--sf-color-action-light) clamp(0.65, calc(0.95 - l * 0.5), 0.88) calc(c * 0.9) h)))", | ||
| "value": "light-dark(var(--sf-color-action-source-light), var(--sf-color-action-source-dark, oklch(from var(--sf-color-action-source-light) clamp(0.65, calc(0.95 - l * 0.5), 0.88) calc(c * 0.9) h)))", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the source-dark group label to the new suffix.
These generated metadata rows still expose (-dark, animatable) even though the public token names were renamed to *-source-dark; this can leak the deprecated suffix into configurator grouping/search. Fix the source metadata and regenerate rather than hand-editing this artifact.
Proposed metadata wording
- "group": "DARK SOURCE TOKENS (-dark, animatable)",
+ "group": "DARK SOURCE TOKENS (-source-dark, animatable)",Also applies to: 1998-2005, 2171-2174, 2702-2709, 3235-3238, 3278-3285, 3427-3430, 3470-3476, 3811-3814, 4342-4349, 4539-4542, 5070-5077, 5291-5294, 5822-5829, 6043-6046, 6086-6093, 6211-6214, 6742-6749, 7323-7326, 7366-7373
🤖 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 `@configurator/src/data/api-index.generated.json` around lines 1467 - 1470, The
group label "DARK SOURCE TOKENS (-dark, animatable)" in the
api-index.generated.json file is outdated and exposes the deprecated suffix.
Since this is a generated artifact, do not edit it directly. Instead, locate the
source metadata configuration that generates this file, update all occurrences
of the group label to reflect the new naming convention and remove the reference
to the old (-dark, animatable) suffix, then regenerate the
api-index.generated.json file to ensure all affected entries (including those at
the line ranges specified in the comment) are updated consistently.
Comment on lines
+3398
to
+3406
| "name": "--sf-color-heading", | ||
| "tier": "PUBLIC", | ||
| "role": "consumption", | ||
| "namespace": "color", | ||
| "category": "Core tokens", | ||
| "group": "DARK SOURCE TOKENS (-dark, animatable)", | ||
| "description": "OKLCH dark source tokens for brand and status colors. Registered as typed <color> so scoped [data-theme=\"dark\"] sections inherit correct absolute values rather than an unresolved var() text string.", | ||
| "note": "Form validation error color. Typically same hue as danger but can be tuned independently.", | ||
| "value": "light-dark(var(--sf-color-error-light), var(--sf-color-error-dark, oklch(from var(--sf-color-error-light) clamp(0.65, calc(0.95 - l * 0.5), 0.88) calc(c * 0.9) h)))", | ||
| "group": "Text", | ||
| "description": "Semantic text colors for every content role. All tokens adapt to light and dark mode automatically.", | ||
| "note": "Heading text color. Slightly stronger contrast than body text for visual hierarchy.", | ||
| "value": "light-dark( oklch(from var(--sf-color-neutral-source-light) clamp(0.05, calc(l - 0.4 - var(--sf-contrast-bias)), 0.35) c h), oklch(from var(--sf-color-neutral) clamp(0.70, calc(l + 0.25 + var(--sf-contrast-bias)), 1) c h) )", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align --sf-color-heading with its stated contrast semantics.
--sf-color-heading currently computes the exact same value as --sf-color-text, but its note promises “slightly stronger contrast.” Either adjust the heading formula or update the note/aliasing to make the semantic contract explicit.
Also applies to: 6910-6918
🤖 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 `@configurator/src/data/api-index.generated.json` around lines 3398 - 3406, The
`--sf-color-heading` token's note claims it has "slightly stronger contrast than
body text for visual hierarchy," but the current value formula is identical to
`--sf-color-text`, making the semantic contract misleading. Either adjust the
oklch() formula values in both the light-dark branches (the clamp constraints or
the arithmetic operations on l, c, h values) to make the heading color produce
noticeably stronger contrast compared to body text, or update the note to
accurately reflect that they are semantically equivalent. Check all other
occurrences of this token around line numbers 6910-6918 to apply the same fix
consistently across the file.
After the dark branch of --sf-color-selection-bg was corrected to use --sf-color-action-source-dark (instead of -source-light), the resolved value changes from oklch(0.5 0.22 235) to its auto-derived dark equivalent oklch(0.7 0.198 235). Update both the light-section light-dark() expression and the dark-section resolved entry.
The [data-theme="dark"] override for --sf-color-selection-bg was still referencing --sf-color-action-source-light. This override takes precedence over the light-dark() fix in tokens.css, so dark-mode selection highlights were drawing from the light source (oklch 0.5 0.22 235) instead of the darker auto-derived source (oklch 0.7 0.198 235). Update to -source-dark for semantic correctness and to match the P8 baseline.
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
2 participants
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.
Three API improvements based on design system feedback:
Source token rename: --sf-color-X-light → --sf-color-X-source-light
(and -dark → -source-dark) for all 6 brand + 4 status families.
Eliminates the naming collision with shade aliases (-lighter, -xlight,
-superlight) that implied a lightness scale when -light was actually
a mode input. Resolved semantic tokens (--sf-color-primary etc.) unchanged.
Merge error → danger: --sf-color-error is now an alias for --sf-color-danger.
danger covers both destructive actions and form-validation errors.
Removes --sf-color-error-source-light/dark as standalone customisation knobs.
All --sf-color-error-* derived tokens (subtle/muted/strong/text--on-error)
kept as backwards-compatible aliases. .is-invalid and .is-error states
updated to reference danger tokens directly.
Add .sf-section--guttered: adds padding-inline: var(--sf-gutter) to a
section, enabling gutter-less layouts without a nested .sf-container.
Migration notes added to docs/migration.md.
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_015eRstwpktwGixBeiKTKhkk
Summary by CodeRabbit
.sf-section--gutteredfor consistent section gutter padding without double-guttering.-light/-darkto-source-light/-source-darkacross the system.