-
Notifications
You must be signed in to change notification settings - Fork 1
feat: detect and warn when step overrides shadow generated scales #663
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
jackgranatowski
merged 3 commits into
main
from
claude/plugin-configurator-controls-sule1f
Jul 28, 2026
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
ec8576e
fix(configurator): track ratio tokens in the scale cards' override state
claude 8becbda
feat(configurator): warn when fixed step values shadow a generated scale
claude ea8b3c7
fix(configurator): correct shadow-notice wording and label its disclo…
claude File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
56 changes: 56 additions & 0 deletions
56
configurator/src/components/inputs/ScaleShadowNotice.svelte
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| <script lang="ts"> | ||
| /** | ||
| * Warns that concrete per-step values in the override map are shadowing the | ||
| * scale knobs on this card, and offers to clear them. See lib/scaleShadow.ts | ||
| * for why this state is otherwise invisible. | ||
| */ | ||
| let { tokens, scaleLabel, onClear }: { | ||
| /** The shadowing step tokens, in ladder order. */ | ||
| tokens: string[]; | ||
| /** e.g. "spacing" — reads as "…override the generated spacing scale". */ | ||
| scaleLabel: string; | ||
| onClear: () => void; | ||
| } = $props(); | ||
|
|
||
| let expanded = $state(false); | ||
| let one = $derived(tokens.length === 1); | ||
| // aria-controls needs a stable id, and two notices (spacing + type) can be | ||
| // mounted at once, so it must be unique per instance rather than a constant. | ||
| const listId = `sf-shadowed-tokens-${crypto.randomUUID()}`; | ||
| </script> | ||
|
|
||
| {#if tokens.length} | ||
| <div class="rounded-lg bg-amber-500/10 border border-amber-500/20 p-2.5 space-y-2"> | ||
| <p class="text-[10px] text-amber-700 dark:text-amber-300 leading-relaxed"> | ||
| {tokens.length} | ||
| {one ? 'token holds a fixed value that overrides' : 'tokens hold fixed values that override'} | ||
| the generated {scaleLabel} scale, so the controls below | ||
| {one ? 'do not affect that step' : 'do not affect those steps'}. | ||
| Fixed values win over the scale that would generate them. | ||
| </p> | ||
| <div class="flex items-center gap-2"> | ||
| <button | ||
| onclick={onClear} | ||
| class="text-[10px] font-semibold text-amber-800 dark:text-amber-200 bg-amber-500/20 hover:bg-amber-500/30 border border-amber-500/30 rounded px-2 py-1 cursor-pointer transition-colors" | ||
| > | ||
| Clear {one ? 'it' : 'them'} and use the scale | ||
| </button> | ||
| <button | ||
| onclick={() => { expanded = !expanded; }} | ||
| aria-expanded={expanded} | ||
| aria-controls={listId} | ||
| aria-label={expanded ? `Hide the ${scaleLabel} tokens holding fixed values` : `Show the ${scaleLabel} tokens holding fixed values`} | ||
| class="text-[10px] text-amber-700/80 dark:text-amber-300/80 hover:underline cursor-pointer" | ||
| > | ||
| {expanded ? 'hide' : 'show'} | ||
| </button> | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| </div> | ||
| {#if expanded} | ||
| <ul id={listId} class="space-y-0.5 pt-0.5"> | ||
| {#each tokens as token (token)} | ||
| <li class="text-[9px] font-mono text-amber-700/80 dark:text-amber-300/80">{token}</li> | ||
| {/each} | ||
| </ul> | ||
| {/if} | ||
| </div> | ||
| {/if} | ||
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| /** | ||
| * Detects overrides that shadow a generative scale. | ||
| * | ||
| * The space and text scales are generated: the framework computes every step | ||
| * (--sf-space-m, --sf-text-l, …) from a handful of source knobs (base, ratio, | ||
| * scale, fluid viewport) via clamp()/pow() at :root. The scale panels edit those | ||
| * knobs, never the steps. | ||
| * | ||
| * A concrete value stored for a STEP wins over the generated one — deliberately: | ||
| * a fine-tuned rung should survive the knob that would otherwise produce it, and | ||
| * both the live preview and the WordPress plugin's PHP emitter implement that | ||
| * same precedence. But when a whole ladder of steps is stored (an older settings | ||
| * page, an imported theme, or hand edits in the All-tokens tab), the knobs go | ||
| * completely inert while still reading back their own values everywhere — the | ||
| * panel shows them, the page reports them at :root, and nothing moves. There is | ||
| * no way to tell from the scale card that this is happening. | ||
| * | ||
| * These helpers let the scale cards say so, and offer to clear the shadowing | ||
| * entries. | ||
| */ | ||
|
|
||
| /** Steps of the fluid space scale, in ladder order. Excludes the deliberately | ||
| * non-generative --sf-space-none / --sf-space-px. */ | ||
| export const SPACE_STEP_TOKENS = [ | ||
| '--sf-space-2xs', '--sf-space-xs', '--sf-space-s', '--sf-space-m', | ||
| '--sf-space-l', '--sf-space-xl', '--sf-space-2xl', '--sf-space-3xl', | ||
| '--sf-space-4xl', | ||
| ] as const; | ||
|
|
||
| /** Steps of the fluid text scale. */ | ||
| export const TEXT_STEP_TOKENS = [ | ||
| '--sf-text-2xs', '--sf-text-xs', '--sf-text-s', '--sf-text-m', | ||
| '--sf-text-l', '--sf-text-xl', '--sf-text-2xl', '--sf-text-3xl', | ||
| '--sf-text-4xl', | ||
| ] as const; | ||
|
|
||
| /** Steps of the display scale, generated from the display base + the shared | ||
| * text ratio. */ | ||
| export const DISPLAY_STEP_TOKENS = [ | ||
| '--sf-text-display-s', '--sf-text-display-m', '--sf-text-display-l', | ||
| ] as const; | ||
|
|
||
| /** | ||
| * Which of `steps` are present in the override map — i.e. stored as concrete | ||
| * values that shadow the generated ones. Order follows `steps`, so the result | ||
| * reads as a ladder rather than in insertion order. | ||
| */ | ||
| export function shadowingSteps( | ||
| overrides: Record<string, string>, | ||
| steps: readonly string[], | ||
| ): string[] { | ||
| return steps.filter((token) => token in overrides); | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| /** | ||
| * Contract test for configurator/src/lib/scaleShadow.ts. | ||
| * | ||
| * The scale panels warn when a concrete per-step value in the override map | ||
| * shadows the generated scale, and offer to clear those entries. That guard is | ||
| * only as good as its list of step tokens: a step the list misses keeps | ||
| * silently overriding the knobs with no warning — the exact failure the guard | ||
| * exists to surface — and a step that no longer exists would offer to "clear" a | ||
| * token nothing reads. | ||
| * | ||
| * The lists are therefore pinned to the source of truth: every token in | ||
| * core/tokens.css whose value is built from the corresponding generator inputs | ||
| * IS a generated step, by definition. Add a rung to the framework (a 5xl, say) | ||
| * and this test fails until the guard learns about it. | ||
| * | ||
| * Runs in the root unit suite (node --test → CI), like | ||
| * tests/configurator-data-contract.test.js, so a TypeScript source in the | ||
| * configurator package is checked without pulling in a TS toolchain: the lists | ||
| * are plain string-literal arrays and are read as text. | ||
| * | ||
| * Run: node --test tests/scale-shadow-steps.test.js | ||
| */ | ||
| import { test, describe } from 'node:test'; | ||
| import assert from 'node:assert/strict'; | ||
| import fs from 'node:fs'; | ||
| import path from 'node:path'; | ||
| import { stripComments } from '../scripts/lib/parse.js'; | ||
|
|
||
| const ROOT = path.resolve(import.meta.dirname, '..'); | ||
| const TOKENS_CSS = path.join(ROOT, 'core', 'tokens.css'); | ||
| const SCALE_SHADOW = path.join(ROOT, 'configurator', 'src', 'lib', 'scaleShadow.ts'); | ||
|
|
||
| /** Every `--sf-*: value;` declaration in core/tokens.css, as [name, value]. */ | ||
| function declarations() { | ||
| const css = stripComments(fs.readFileSync(TOKENS_CSS, 'utf8')); | ||
| const out = []; | ||
| for (const line of css.split('\n')) { | ||
| const m = line.match(/^\s*(--sf-[a-z0-9-]+)\s*:\s*(.+);\s*$/); | ||
| if (m) out.push([m[1], m[2]]); | ||
| } | ||
| // Guard the parse itself: if the file's formatting ever changes so that | ||
| // declarations stop being one-per-line, every set below would come back empty | ||
| // and each assertion would vacuously pass. | ||
| assert.ok(out.length > 100, `parsed only ${out.length} declarations from core/tokens.css — parser is stale`); | ||
| return out; | ||
| } | ||
|
|
||
| /** Tokens whose value is generated from `input` (e.g. --sf-space-base-min). */ | ||
| function generatedFrom(decls, input) { | ||
| return decls.filter(([, value]) => value.includes(`var(${input})`)).map(([name]) => name).sort(); | ||
| } | ||
|
|
||
| /** Read an exported string-literal array out of the TypeScript source. */ | ||
| function listFromSource(source, exportName) { | ||
| const m = source.match(new RegExp(`export const ${exportName} = \\[([\\s\\S]*?)\\]`)); | ||
| assert.ok(m, `${exportName} not found in scaleShadow.ts`); | ||
| return [...m[1].matchAll(/'(--sf-[a-z0-9-]+)'/g)].map((x) => x[1]).sort(); | ||
| } | ||
|
|
||
| describe('scaleShadow step lists match the generated scales', () => { | ||
| const decls = declarations(); | ||
| const source = fs.readFileSync(SCALE_SHADOW, 'utf8'); | ||
|
|
||
| // The display steps are generated from --sf-text-display-base-min AND read the | ||
| // shared --sf-text-ratio-*; the plain text steps use --sf-text-base-min. Both | ||
| // sets are disjoint because the base inputs differ. | ||
| const cases = [ | ||
| ['SPACE_STEP_TOKENS', '--sf-space-base-min'], | ||
| ['TEXT_STEP_TOKENS', '--sf-text-base-min'], | ||
| ['DISPLAY_STEP_TOKENS', '--sf-text-display-base-min'], | ||
| ]; | ||
|
|
||
| for (const [exportName, input] of cases) { | ||
| test(`${exportName} covers exactly the tokens generated from ${input}`, () => { | ||
| const generated = generatedFrom(decls, input); | ||
| assert.ok(generated.length > 0, `no tokens in core/tokens.css are generated from ${input}`); | ||
| assert.deepEqual( | ||
| listFromSource(source, exportName), | ||
| generated, | ||
| `${exportName} is out of sync with core/tokens.css — the shadow guard would ` + | ||
| 'either miss a step (no warning, knobs silently inert) or offer to clear a dead token', | ||
| ); | ||
| }); | ||
| } | ||
|
|
||
| test('the non-generative spacing tokens are excluded', () => { | ||
| // --sf-space-none / --sf-space-px are fixed values, not rungs of the scale: | ||
| // overriding them cannot shadow the knobs, so offering to "restore the | ||
| // scale" for them would be wrong. | ||
| const list = listFromSource(source, 'SPACE_STEP_TOKENS'); | ||
| assert.ok(!list.includes('--sf-space-none')); | ||
| assert.ok(!list.includes('--sf-space-px')); | ||
| }); | ||
| }); |
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.
Uh oh!
There was an error while loading. Please reload this page.