CORE-2720: Add a global CSS token file generated from the theme - #150
CORE-2720: Add a global CSS token file generated from the theme#150OpenStaxClaude wants to merge 9 commits into
Conversation
Replaces the hand-copied literals in the 22 stylesheets migrated before the tokens existed with token references, and empties PENDING_SWEEP so the duplicate-literal check now covers every stylesheet in src/. The --component-* override hooks are unchanged. Only their defaults moved, from a JS inline style to the CSS side: var(--tabs-active-border-color, var(--ox-color-dark-green)) Static bindings dropped from Tooltip, ButtonBar, Tabs, Toast, ToastContainer, NavBar, NavBarMenuButtons, Radio, Checkbox, TreeCheckbox and ButtonLink — 44 of them across 19 components, each existing only to push a static palette colour through an inline style on every render. Dynamic bindings stay in JS: the button and checkbox variant lookups, navbar height / maxWidth / justifyContent, disabled opacity, and the dropdown caret colour. Behaviour note. Removing an inline default changes cascade precedence. A consumer setting e.g. --tabs-border-color on an ancestor element via their own stylesheet used to lose to the inline default; now it applies. Passing the variable through the component's own style prop — the documented path, and the one the specs cover — is unaffected. Specs that asserted the inline default were rewritten to assert the thing that matters, that a caller override still works. tokens.spec.ts now guards the defaults themselves. Fixes a latent bug in passing: --button-shadow is shared by Button.css and DropdownMenu.css and the JS binds palette.black for all three variants, but Button's fallback had drifted to #424242 while DropdownMenu's was #000000. Unreachable today because the JS always sets it, so no visual change — but the two would have rendered the same variant differently if it were ever reached. Genuinely off-palette values (#ccc, #ddd) are left alone on the allowlist with a reason rather than snapped to the nearest palette entry. That is a design decision, not a refactor. Includes the Button.css and DropdownMenu.css half of Roy's cbf780d, which renames the button variant tokens. It cannot be a separate commit on this branch: it edits token references inside already-swept CSS, so it has nothing to apply to until this commit lands. The theme half is preserved as his own commit on #150. Roy's 463bb86 — dropping a needless style-prop rename in ButtonBar — is likewise folded in rather than reverted and reapplied. Split out of the original #143. Stacked on #150; the resulting tree is byte-identical to the reviewed #143 head (7f8bdfb content), so the split carries no change of its own. Co-authored-by: Roy Johnson <roy.e.johnson@rice.edu> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Establishes one place a theme value is written and referenced from CSS, with
a test that fails if the two disagree. The sweep of the already-migrated
stylesheets onto it follows separately.
src/theme/theme.css holds a single :root block. Colour tokens are the
kebab-case palette key (palette.neutralLighter -> --ox-color-neutral-lighter)
plus --ox-color-link, --ox-color-link-hover, --ox-z-index-* and
--ox-padding-navbar-*. The --ox- prefix avoids collisions with a consuming
app's own variables.
The file is generated, not hand-written. themeCss.ts owns the projection and
npm run generate:theme-css writes it; build.bash runs it as its first step,
before either tsc pass and before the rsync, and publish.bash inherits that
via build:clean, so a published package cannot ship a stale file. It is
committed as well as generated because jest and ladle read src/ directly and
CI runs lint/test rather than build. Deliberately not hooked into pretest —
regenerating before the suite would make the freshness check pass vacuously.
Adds the four button variant colours to the palette, which theme/buttons.ts
had been holding as bare string literals with nothing recording that they
are hover/active variants of orange and darkGray. Purely additive.
Enforcement, in tokens.spec.ts, on top of the CORE-2736 engine:
1. The committed theme.css is exactly what the generator produces. One
equality, so a missing token, an orphan token and a stale value all
fail the same way.
2. No component stylesheet writes a colour literal that duplicates a
theme value.
3. No component stylesheet introduces a colour that is neither a theme
value nor on the KNOWN_OFF_PALETTE allowlist, each entry with a reason.
4. No component stylesheet reads an --ox-* token that does not exist,
which would otherwise fall through to its fallback silently.
Check 2 cannot pass yet — 16 stylesheets migrated before the tokens existed
still carry hand-copied literals. PENDING_SWEEP names them, and is asserted
to be exactly the failing set so it cannot rot in either direction: dropping
a name without sweeping the file fails, and sweeping a file without dropping
its name fails too. The list reaches empty in the sweep PR and goes away
with the assertion.
No CSS @import: build.bash rsyncs CSS 1:1 with no bundler, so an @import
would depend on each consumer's resolver. Component .tsx files import
theme.css alongside their own stylesheet instead. Consumers need do nothing.
Split out of #143. Stacked on #149.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The "what CI enforces" list read as though the duplicate-literal check covered every stylesheet, which it will not until the sweep lands. Says which files are exempt, why the list cannot drift, and that new stylesheets are not to be added to it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PENDING_SWEEP exempted a file from every colorProblems finding, not only the
duplicate literals it was listed for. The exactness assertion only asked whether
each file had at least one problem, so a new off-palette colour added to an
already-failing file left the failing set unchanged and the suite passed —
enforcement rule 3 had a hole exactly where the migrated stylesheets are.
colorProblems now returns { duplicates, offPalette }. The off-palette check runs
over every stylesheet; PENDING_SWEEP defers duplicates only, and the exactness
assertion is over the duplicate findings. Verified both directions: appending
`color: #ab12cd` to Button.css passed on the previous spec and fails on this one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
f74f4fe to
8ed36c2
Compare
CSS function names are ASCII case-insensitive, so `VAR(--ox-color-palee)` is a real reference to a token nothing defines. The regex only knew the lowercase spelling, so that typo fell through to its fallback unreported — precisely what check 4 exists to catch. The lookup stays case-sensitive: custom property names are, so `var(--OX-color-pale)` is genuinely undefined and is now reported rather than skipped. Both spellings are covered by tests, which fail against the old regex. The colour checker itself was already sound here — cssColors lowercases function names before dispatching — but `VAR()` cases are added alongside the lowercase ones so that stays pinned from this file too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot is right on both halves. Nothing on this branch imports theme.css — no stylesheet reads a token until the sweep in #143 — and the `./*` export pattern rewrote every subpath to `*.js`, so `@openstax/ui-components/theme/ theme.css` resolved to `theme.css.js` and threw. As published, the tokens were unreachable by either route. The export map now carries a `./*.css` pattern ahead of `./*`, so CSS subpaths resolve to the real files in both trees. Verified against a built dist with node's own resolver: the CSS subpath resolves to dist/cjs under require and dist/esm under import, and JS subpaths are unaffected. The import itself is now a rule the suite enforces rather than a line in the README: a stylesheet that reads a token whose importing module does not import theme.css fails. Proved by making Loader.css read a token — the test fails until Loader.tsx imports the token file. That is what stops the sweep in #143 shipping a stylesheet whose var() silently takes its fallback, which looks right on screen because the fallback is the literal the token replaced. README documents the direct entry point for an app that wants the tokens without rendering one of our components. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
The import scan was a regex over the source, so `// import '../theme/theme.css';` counted as an import. That fails the wrong way round for this check: it reports the token file as present when it is absent, so deleting a real import and leaving a commented-out one beside it would pass. Collects the module's ImportDeclaration nodes through the TypeScript parser instead, with the script kind chosen from the extension since TS and TSX disagree about `<T>x`. Comments and string contents are no longer imports. Four cases added: a commented-out import in both syntaxes, the specifier inside a string, a real import with a trailing comment, and a .tsx module. The first two fail against the regex. Not deferred to CORE-2885 — that ticket is the CSS lexer, this is TypeScript, and the parser is already a devDependency, so the exact answer costs less than the approximation did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as low quality.
This comment was marked as low quality.
There was a problem hiding this comment.
🔵 Needs a closer look
The unknown-token check can overlook valid non-ASCII custom-property names by truncating them to existing tokens.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/theme/tokens.spec.ts:183
- This character class truncates valid non-ASCII custom-property names. For example,
var(--ox-color-redé)is captured as--ox-color-red; because that token exists, the undefined--ox-color-redéreference passes the check. Capture the full CSS identifier so the lookup sees the actual referenced name, and add this as a regression case.
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
RoyEJohnson
left a comment
There was a problem hiding this comment.
See Copilot's latest comment. It may fall under 'do it later with the tokenizer'.
`[\w-]+` stopped at the first character outside ASCII and handed back a prefix, so `var(--ox-color-redé)` was looked up as `--ox-color-red`, found, and passed. CSS name code points include everything at U+0080 and above, so that is a valid name and a different one — the check reported an undefined reference as fine, which is the single answer it must never give. The class now runs to the end of the identifier. Regression cases for a combining-accent name, an astral one (surrogate pair), a non-ASCII character in the middle, and the prefix it truncated to, which still passes. The first three return [] against the old class. Escaped spellings are untouched and now documented where the regex is: they read literally, so `var(--ox-color-r\65 d)` is reported as undefined. That is the decode-the-escapes work in CORE-2885 rather than a boundary this regex can fix, and it errs toward a failure rather than a pass. Noted on the ticket. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@RoyEJohnson — I split it, and the two halves fall on opposite sides of your line. Fixed here ( Deferred to CORE-2885, exactly as you suggested: escaped spellings. It also errs in the safer direction — a false positive fails CI on valid CSS, which gets noticed, where the non-ASCII one passed silently. That asymmetry is why I did not hold both for the ticket. 726 tests / 41 suites / 114 snapshots pass; typecheck, lint and |
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate review findings in src/theme/tokens.spec.ts remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
src/theme/tokens.spec.ts:132
- The allowlist is checked before the theme lookup, so promoting an allowlisted value into
palette.tsleaves every existing#ccc/#dddliteral silently exempt: it never reachesduplicatesandPENDING_SWEEPcannot require the migration. Since the README explicitly supports moving an allowlisted color into the palette, only honor this exemption when the channels are still absent fromthemeValues(and add a regression case for that transition).
if (KNOWN_OFF_PALETTE.has(key)) { continue; }
src/theme/tokens.spec.ts:195
- The regex is not bounded to the
varfunction name, so it also matches thevar(suffix of a different function such asmyvar(--ox-color-palee). That is a valid token stream in a custom-property value and does not read a CSS variable, but this check reports an unknown token. Add an identifier boundary (and a regression case); apply the same boundary toreadsThemeTokenbelow.
[...stripNoise(css).matchAll(/var\(\s*(--ox-(?:[\w-]|[^\x00-\x7f])+)/gi)]
src/theme/tokens.spec.ts:507
- This has the same unbounded
var(match:myvar(--ox-color-pale)makes the import rule believe the stylesheet reads a theme token even though it does not. A component using such a custom-property value would be forced to importtheme.cssunnecessarily; use the same CSS identifier boundary as the token-reference scan.
const readsThemeToken = (css: string) => /var\(\s*--ox-/i.test(stripNoise(css));
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
| */ | ||
| const unknownTokenReferences = (css: string, defined: Map<string, string>) => [ | ||
| ...new Set( | ||
| [...stripNoise(css).matchAll(/var\(\s*(--ox-(?:[\w-]|[^\x00-\x7f])+)/gi)] |
Jira: CORE-2720
2 of 3 — split of #143.
main→ #149 → #150 → #143.What this is
The design decision: one place a theme value is written, referenced from CSS, with a test that fails if the two disagree. The sweep of the already-migrated stylesheets onto it is #143.
src/theme/theme.cssholds a single:rootblock. Color tokens are the kebab-case palette key (palette.neutralLighter→--ox-color-neutral-lighter), plus--ox-color-link,--ox-color-link-hover,--ox-z-index-*and--ox-padding-navbar-*. The--ox-prefix avoids collisions with a consuming app's own variables.The file is generated, not hand-written.
themeCss.tsowns the projection;npm run generate:theme-csswrites it.build.bashruns it as its first step, before eithertscpass and before the rsync, andpublish.bashinherits that viabuild:clean, so a published package cannot ship a stale file. It is committed as well as generated because jest and ladle readsrc/directly and CI runs lint/test rather than build. Deliberately not hooked intopretest— regenerating before the suite would make the freshness check pass vacuously.Adds the four button variant colors to the palette.
theme/buttons.tshad been holding them as bare string literals with nothing recording that they are hover/active variants oforangeanddarkGray. Purely additive to the published palette.Enforcement
tokens.spec.ts, on top of #149's engine, checks four things:theme.cssis exactly what the generator produces. One equality, so a missing token, an orphan token and a stale value all fail the same way.KNOWN_OFF_PALETTEallowlist, each entry carrying a reason. The README says what the check covers, what it excludes on purpose (a bare name outside a color context —animation-name: redis not a color) and the one known gap (escaped property and function names, CORE-2885).--ox-*token that does not exist — a typo would otherwise fall through to its fallback silently. Matched case-insensitively, since CSS function names are (VAR(--ox-color-palee)is a real reference); the lookup itself stays case-sensitive, since property names are, sovar(--OX-color-pale)is reported too. The name runs to the end of the CSS identifier, non-ASCII included —[\w-]+truncatedvar(--ox-color-redé)to a token that exists and passed it. Escaped spellings still read literally and are deferred to CORE-2885 with the rest of the escape decoding; that one errs toward a CI failure on valid CSS rather than a silent pass.The one wrinkle in the split
Check 2 cannot pass yet: 16 stylesheets migrated before the tokens existed still carry hand-copied literals, and they are not swept until #143.
PENDING_SWEEPnames them.The list is asserted to be exactly the failing set, so it cannot rot in either direction — dropping a name without sweeping the file fails, and sweeping a file without dropping its name fails too. Verified both ways. It reaches empty in #143 and is deleted there along with the assertion. The README says the same, so nobody adds to it.
This is the cost of the split and the only thing in these three PRs that would not exist without it. If you'd rather not carry it, the alternative is landing #150 and #143 together.
No CSS
@import, and how the file gets loadedbuild.bashrsyncs CSS 1:1 with no bundler, so an@importwould depend on each consumer's resolver, and a stylesheet does not dragtheme.cssin by itself. The component that imports the stylesheet imports the token file alongside it.Nothing does that yet on this branch, correctly: no stylesheet reads a token until #143 sweeps one, so the tokens are defined and unused. Rather than write the imports early, the rule is enforced —
tokens.spec.tsfails when a stylesheet reads a token and the module importing it does not importtheme.css. Verified it fires by makingLoader.cssread one. The per-file assertion is vacuous on this branch by construction and says so; the helper under it is tested on synthetic input.Imports are read from the module's
ImportDeclarationnodes via the TypeScript parser, not matched in the source. A regex also sees// import '../theme/theme.css';, which fails the wrong way round for this check — it reports the token file present when it is absent, so deleting a real import beside a commented-out one would pass.The export map also carried a real bug, found in review:
"./*"rewrote every subpath to*.js, so@openstax/ui-components/theme/theme.cssresolved totheme.css.jsand threw. A./*.csspattern now sits ahead of it, verified with node's own resolver against a built dist —dist/cjsunderrequire,dist/esmunderimport, JS subpaths unaffected. That would have outlived the split: without it a consumer could not load the tokens directly even after #143.Verification
build:cleanclean.PENDING_SWEEPscaffolding.Provenance
Roy's
cbf780d(rename the button variant colors) is preserved as his own commit, theme side. TheButton.css/DropdownMenu.csshalf moves to #143 — it edits token references inside already-swept CSS, so it has nothing to apply to until that lands.theme.cssis regenerated there rather than hand-edited.Earlier review rounds on #143 (build integration, the raw-data-file alternative, the
as stringcasts, the deadtokenisedflag,parseHexgrammar, the property-context gate) are all carried across — see the CORE-2720 ticket for the full record.🤖 Generated with Claude Code