Repository navigation
Keep light and dark --om-* tokens in tokens.css only - #35294
shah-harshit wants to merge 4 commits into
Conversation
The dark values for the legacy --om-* aliases lived in app.less while their light values lived in tokens.css, so import order decided which won (#34936 made light win everywhere). Move the dark block next to the light values, drop a dead duplicate @Grey-25, and guard the rule in theme-contract.test.js. Also clears the existing token-drift failures (stale generated block, link role expectation, broken card spec link) so the hook can gate on it.
shah-harshit
added
UI
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| test('gives every dark --om-* override a light value in tokens.css', () => { | ||
| const css = fs.readFileSync(TOKENS_FILE, 'utf8'); | ||
| const dark = declarations(extractBlock(css, ':root.dark-mode')); | ||
| const light = declarations(extractBlock(css, ':root {')); | ||
|
|
||
| assert.ok(dark.size > 0); | ||
| for (const token of dark.keys()) { | ||
| assert.ok(light.has(token), `${token} has a dark value but no light one`); | ||
| } | ||
| }); |
There was a problem hiding this comment.
⚠️ Bug: Dark-override guard parses the light :root block, so it always passes
extractBlock(css, ':root.dark-mode') uses indexOf. The first match is the header comment this PR adds in tokens.css at line 32 (:root.dark-mode — both here…), not the rule at line 386. The next { after that comment opens the light :root { block at line 45, so dark and light end up as the same map. The test 'gives every dark --om-* override a light value' checks the light block against itself and can never fail. A dark-only token added at line 386 would get through, even though the PR says it tested exactly that case. Fix: anchor the marker to a real rule, e.g. ' :root.dark-mode {' or a regex like /^:root\.dark-mode\s*\{/m. Also assert that the dark map contains a known dark token such as --om-grey-15 with value var(--color-border-subtle).
Anchor the marker to the rule, not the comment, and check that the right block was parsed:
const dark = declarations(extractBlock(css, '
:root.dark-mode {'));
const light = declarations(extractBlock(css, '
:root {'));
assert.equal(dark.get('--om-grey-15'), 'var(--color-border-subtle)');
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
Jest test CoverageUI tests summary
|
main added --om-space-17, --om-space-70 and --om-space-220, which nothing references, so the generated block in tokens.css and token-reference.md drifted again. Regenerate so check-drift passes.
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source
|
This branch has not been deployed
1 participant
Describe your changes:
Fixes open-metadata/openmetadata-collate#7326
Root cause: #34936 added light values for the legacy
--om-*aliases on:rootintokens.css. Their dark values lived under.dark-modeinapp.less.tokens.cssis imported afterapp.less, and both selectors have the same specificity, so the light values won. #35271 patched the selector to:root.dark-mode, but each legacy colour was still defined in three files:variables.less(with its fallback),tokens.css(light) andapp.less(dark). Correctness depended on import order across files.Change: one source file per token layer.
:root.dark-mode { --om-* }block fromapp.lessintotokens.css, directly after the light values.app.lesskeeps a pointer comment only.@grey-25invariables.less. LESS keeps the last definition (#fdfdfd), so the earlier#dbe0e7line was unused and its fallback disagreed withtokens.css. No behaviour change.theme-contract.test.js, run byyarn token-driftand its pre-commit hook):tokens.cssdeclares an--om-*property (allowlist:--om-page-height, a per-shell layout value);--om-*override has a light value intokens.css;variables.lessbridge is defined once, and its fallback equals thetokens.csslight value.ADR:2026-10-13-om-tokens-have-one-source-file, plus a rule in.claude/rules/frontend-styling.md.main, fixed so the hook can actually gate:tokens.cssandtoken-reference.md: regenerated, which only prunes unreferenced legacy tokens;#53B1FD);card.mdlinked tocard.less, deleted by the Card migration.Values and selectors are unchanged (
:root.dark-modestill out-ranks:root), so light and dark render as before.Also in this PR (from #35395):
docs/colors.mddocumented stale dark values for two brand tokens. They now match coreglobals.css:bg-brand-primaryis brand-600 at 20% (was "brand-500 at 14%" /#2e90fa) andborder-brandis brand-600#1570ef(was brand-400#53b1fd). Docs only.Merged
main:mainadded--om-space-17,--om-space-70and--om-space-220, which nothing references, so the generated block drifted again; regenerated socheck-driftpasses.Type of change:
High-level design:
Core tokens (
--color-*,--tw-*) stay in coreglobals.css. Project--om-*tokens go intokens.cssonly, light on:rootand dark on:root.dark-mode. The.dark-mode bodybrand-accent block inapp.lessis out of scope: it overrides--tw-*(not--om-*) to beat inline brand vars on<html>, and the ADR records why it stays.Tests:
Use cases covered
--om-*inapp.lessfails the guard. I checked this by injecting.dark-mode { --om-grey-15: red }; the test failed, and passes again after removal.variables.lessfallback fails.Unit tests
node scripts/design-tokens/check-drift.jsreports no drift after mergingmain(a4d9075c750). All 6 steps pass, including the 13 theme-contract tests; onmain, 3 steps fail.node scripts/token-audit.json the changed stylesheets: 0 errors.Backend integration tests
Ingestion integration tests
Playwright (UI) tests
Manual testing performed
None. This is a CSS move with identical values and selectors.
UI screen recording:
Not applicable: no visual change (token move plus guard).
Checklist:
Fixes …above.🤖 Generated with Claude Code