Add light theme support and theme application layer - #6
Closed
jackgranatowski wants to merge 4 commits into
Closed
Conversation
…eme] - Add --sf-color-scheme: dark/light to [data-theme="dark"/"light"] blocks in core/tokens.css so light-dark() resolves correctly in themed subtrees - Add new [data-theme="light"] block with explicit light-mode token resets - Create core/themes.css under @layer slashed.themes: applies color-scheme, background-color, and color to every [data-theme] element so mixed dark/light sections work without consumer-side workarounds - Add core/themes.css to bundle.config.json; rebuild dist/slashed.essential.css https://claude.ai/code/session_01UJGjZdau27VkRjc35wNNdd
|
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 ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR rewires the SLASHED design token system from fixed light-mode neutrals with separate dark-mode overrides to a palette-driven architecture. Semantic tokens now derive from primitives via ChangesTheme tokens and palette system
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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 |
…eeded The [data-theme="light"] block only needs to set --sf-color-scheme: light. With themes.css applying color-scheme: var(--sf-color-scheme) to [data-theme], light-dark() at :root automatically resolves to light values — no token repetition required. https://claude.ai/code/session_01UJGjZdau27VkRjc35wNNdd
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tokens.css`:
- Around line 640-642: The [data-theme="light"] block only sets
--sf-color-scheme and thus inherits dark tokens from ancestors; update the
[data-theme="light"] rule to explicitly define the full set of color custom
properties (e.g., --sf-color-bg, --sf-color-text, --sf-color-border and all
other --sf-... tokens present in the [data-theme="dark"] block) mirroring the
structure and values used in the dark palette (lines defining --sf-... in the
[data-theme="dark"] section) so nested elements with data-theme="light" get a
complete light token palette rather than inheriting dark values.
🪄 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: ad5a67d7-7f6a-4bdd-965f-396387b6864b
⛔ Files ignored due to path filters (1)
dist/slashed.essential.cssis excluded by!**/dist/**
📒 Files selected for processing (1)
core/tokens.css
…x duplication Add --sf-palette-* primitive tokens (36 colors covering slate, indigo, violet, cyan, green, yellow, red, blue). Every semantic token now references palette vars instead of raw hex — each color value is defined exactly once. Semantic tokens in :root use light-dark(paletteRef, paletteRef) for automatic color-scheme response. Non-@Property tokens (surfaces, text, borders, links) cascade correctly through DOM without re-declaration. @Property typed brand tokens (--sf-color-primary, --sf-color-action) are re-declared in [data-theme] blocks because @Property computed values don't cascade light-dark() through the DOM tree. Remove @media (prefers-color-scheme: dark) block entirely — light-dark() with color-scheme: light dark on :root handles system preference natively. Reduce [data-theme="dark"] to: --sf-color-scheme + 2 @Property token re-declarations. [data-theme="light"] same structure — supports nested light-in-dark sections correctly. https://claude.ai/code/session_01UJGjZdau27VkRjc35wNNdd
[data-theme] { color-scheme } → @layer slashed.tokens in core/tokens.css
Activating light-dark() resolution is part of the token mechanism.
[data-theme] { background-color; color } → @layer slashed.base in core/base.css
Mirrors existing body { background-color; color } — same layer, same concern.
Delete core/themes.css; remove from bundle.config.json.
@layer slashed.themes stays declared in core/layers.css for optional future use.
https://claude.ai/code/session_01UJGjZdau27VkRjc35wNNdd
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tokens.css`:
- Around line 500-521: The status color triplets
(--sf-status-success-bg/text/border, --sf-status-warning-*, --sf-status-error-*,
--sf-status-info-*, --sf-status-danger-*) are hard-coded to light-palette values
and will be illegible in dark-themed subtrees; wrap each triplet in light-dark()
to provide a dark-mode alternative (use a stronger/darker hue or a color-mix for
bg fallbacks and adjusted text/border variants for contrast). Update each
variable assignment to use light-dark(lightValue, darkValue) where darkValue is
computed (e.g., color-mix or a mid-tone like --sf-palette-*-500/200 as
appropriate) so nested themed sections render with proper contrast. Ensure you
apply this change to all listed variables (--sf-status-success-*,
--sf-status-warning-*, --sf-status-error-*, --sf-status-info-*,
--sf-status-danger-*) so the PR’s nested theme support is consistent.
🪄 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: 9908af9b-c1e2-4eb2-9007-f87f867d2ef1
⛔ Files ignored due to path filters (1)
dist/slashed.essential.cssis excluded by!**/dist/**
📒 Files selected for processing (1)
core/tokens.css
Comment on lines
500
to
+521
| /* ---------------------------------------------------------- | ||
| Status triplets | ||
| Status triplets (light-only; no dark override in original) | ||
| ---------------------------------------------------------- */ | ||
| --sf-status-success-bg: #f0fdf4; | ||
| --sf-status-success-text: #15803d; | ||
| --sf-status-success-border: #bbf7d0; | ||
| --sf-status-success-bg: var(--sf-palette-green-50); | ||
| --sf-status-success-text: var(--sf-palette-green-700); | ||
| --sf-status-success-border: var(--sf-palette-green-200); | ||
|
|
||
| --sf-status-warning-bg: #fffbeb; | ||
| --sf-status-warning-text: #92400e; | ||
| --sf-status-warning-border: #fde68a; | ||
| --sf-status-warning-bg: var(--sf-palette-yellow-50); | ||
| --sf-status-warning-text: var(--sf-palette-yellow-900); | ||
| --sf-status-warning-border: var(--sf-palette-yellow-200); | ||
|
|
||
| --sf-status-error-bg: #fef2f2; | ||
| --sf-status-error-text: #b91c1c; | ||
| --sf-status-error-border: #fecaca; | ||
| --sf-status-error-bg: var(--sf-palette-red-50); | ||
| --sf-status-error-text: var(--sf-palette-red-700); | ||
| --sf-status-error-border: var(--sf-palette-red-200); | ||
|
|
||
| --sf-status-info-bg: #eff6ff; | ||
| --sf-status-info-text: #1d4ed8; | ||
| --sf-status-info-border: #bfdbfe; | ||
| --sf-status-info-bg: var(--sf-palette-blue-50); | ||
| --sf-status-info-text: var(--sf-palette-blue-700); | ||
| --sf-status-info-border: var(--sf-palette-blue-200); | ||
|
|
||
| --sf-status-danger-bg: #fef2f2; | ||
| --sf-status-danger-text: #b91c1c; | ||
| --sf-status-danger-border: #fecaca; | ||
| --sf-status-danger-bg: var(--sf-palette-red-50); | ||
| --sf-status-danger-text: var(--sf-palette-red-700); | ||
| --sf-status-danger-border: var(--sf-palette-red-200); |
There was a problem hiding this comment.
Status triplets use light-mode-only colors.
These status background/text/border triplets use fixed light palette values (e.g., green-50, yellow-50) which may cause poor contrast or illegibility when used within dark-themed sections. The comment acknowledges "light-only; no dark override in original" but this contradicts the PR's goal of supporting nested themed subtrees.
Consider wrapping these in light-dark():
--sf-status-success-bg: light-dark(var(--sf-palette-green-50), color-mix(in srgb, var(--sf-palette-green-500) 15%, transparent));🤖 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/tokens.css` around lines 500 - 521, The status color triplets
(--sf-status-success-bg/text/border, --sf-status-warning-*, --sf-status-error-*,
--sf-status-info-*, --sf-status-danger-*) are hard-coded to light-palette values
and will be illegible in dark-themed subtrees; wrap each triplet in light-dark()
to provide a dark-mode alternative (use a stronger/darker hue or a color-mix for
bg fallbacks and adjusted text/border variants for contrast). Update each
variable assignment to use light-dark(lightValue, darkValue) where darkValue is
computed (e.g., color-mix or a mid-tone like --sf-palette-*-500/200 as
appropriate) so nested themed sections render with proper contrast. Ensure you
apply this change to all listed variables (--sf-status-success-*,
--sf-status-warning-*, --sf-status-error-*, --sf-status-info-*,
--sf-status-danger-*) so the PR’s nested theme support is consistent.
Contributor
Author
There was a problem hiding this comment.
Skipping — pre-existing gap, not introduced by this PR.
The original core/tokens.css (before any changes in this PR) had these status triplets as light-only hardcoded hex values, and neither the @media (prefers-color-scheme: dark) block nor the [data-theme="dark"] block ever included overrides for them. This PR preserved that behaviour — the only change was converting hardcoded hex to palette references (var(--sf-palette-green-50) etc.).
Adding dark mode variants for status triplets is a valid improvement but requires deliberate design decisions (contrast ratios, colour choices for dark success/warning/error/info states) that are out of scope for this architectural PR. Would be better addressed in a dedicated follow-up.
Generated by Claude Code
There was a problem hiding this comment.
@jackgranatowski, that's a fair point — this PR's only change to those tokens was replacing hardcoded hex with palette references, which is purely mechanical. The dark-mode gap existed before and isn't worsened here.
Would you like me to open a follow-up GitHub issue to track adding light-dark() variants for the status triplets (--sf-status-success-*, --sf-status-warning-*, --sf-status-error-*, --sf-status-info-*, --sf-status-danger-*) so it doesn't get lost?
jackgranatowski
pushed a commit
that referenced
this pull request
May 27, 2026
Resolves the 10 concerns surfaced in the design-level review of PR #130 (stored at semantic-review/2026-05-27-233854-pr-130.md, gitignored). #1 Migrate-mode silent data loss on existing-class collision - Added validateMigrate() pre-validation pass that runs BEFORE any mutation. Detects three cases: a) Target class doesn't exist → safe (creates with seed). b) Target class exists, no key overlap → safe (additive merge). c) Target class exists, same key with different value → CONFLICT. Hard-errors before any mutation, with a message naming the conflicting keys and suggesting Add mode or a different name. - Per-op merge logic: when target exists with no conflicts, missing seed keys are added to its settings before upsert. upsertGlobalClass still never overwrites — we only ADD keys it didn't have. The 'never overwrite' policy is preserved (overwrite = replacing existing values; this is purely additive). - End result: migrate is now safe in all three cases. removeMigratedKeys is only called after every key is guaranteed to be provided by the (new or existing) class with the same value. #2 Modifier-mode auto-numbering rejected legitimate multi-row applies - Two siblings each producing card__image--lg is the canonical 'attach this modifier to all of them' case, NOT a collision. Replaced the blanket name.includes('--') reject with a per-mode check: in modifier mode, intra-plan duplicates are intentional, no error and no numbering. upsertGlobalClass dedupes by name so all rows share the single class. #3 + #10 'label' provenance not honored as authoritative - Added AUTHORITATIVE_PROVENANCE = new Set(['user', 'label']) and use it consistently in applyAutoNumbering. Both user-typed and structure-panel-label-derived names are treated as authoritative and never auto-renumbered. Two authoritative rows colliding is a hard error (mentions row count, not provenance details, since the user shouldn't need to know about reBEMer's internal vocabulary). - Updated apply.js JSDoc typedef to enumerate all five provenance values: 'user' | 'label' | 'element-type' | 'fallback' | 'auto-number'. - §0 status table no longer overstates this row. #4 Auto-numbering didn't re-check post-numbered names - Added a final post-numbering integrity pass: in non-modifier modes, every op must have a unique finalClass after numbering. Catches the case where a user-typed card__image-1 collides with an auto-numbered card__image-1 from a different group. Modifier mode is exempt (duplicates are by design). #5 Badge dataset flag never cleared on host disconnect - Removed the ATTACHED_FLAG dataset entirely. refreshBadges() now uses badgeInstances map state as the source of truth, with two explicit passes: reap stale (host disconnected), then mount any <li> without a live host inside it. The li.contains(existing.host) check handles the common Bricks pattern of rerendering inner .structure-item without removing the outer <li>. - Eliminates the permanent-skip bug where a row that was rendered once and then had its inner subtree rebuilt by Bricks would lose its badge forever. #6 'Use existing' hint not migrate-aware - Row.svelte recommendation copy now branches on mode. Migrate-mode copy reflects the new validateMigrate semantics: 'On Apply, missing style keys will be merged into it. Conflicting values block the migration — pick a different name or use Add.' - Non-migrate modes keep the original 'attach the existing class instead of creating a duplicate' copy. #7 Auto-numbered names didn't refresh the 'use existing' hint - Refactored apply.js to expose a pure buildPlan({ rootId, rows, mode }) → { ok, ops, error }. Returns the ops with their POST-numbering finalClass values. - BemPanel.svelte computes a previewClassNames Map that runs buildPlan and indexes the result by row id. Reactive — re-runs when rows or mode change. - Row.svelte now accepts finalClassName as a prop (from the panel's preview map) instead of computing candidateClassName locally. The 'use existing' hint matches what apply.js will actually produce, even after auto-numbering. - Same buildPlan is used by applyToSubtree, so preview and apply can never disagree. #8 Unused-classes scan was non-deterministic past the cap - Added ORDER BY meta_id ASC to the postmeta query in class-rebemer-rest.php. The cap+1 truncation detection now pairs with reproducible content: two truncated runs on the same site return the same first 5000 rows. #9 Apply has no mid-apply rollback (acknowledged spec-only) - Snapshot/rollback transactional apply (§10) remains spec-only, but the partial-failure path now (a) console.warn-logs the failure with the count of completed mutations, and (b) surfaces a partial-success toast: 'Applied to N elements before halting: <error>. State is partially applied — undo via Bricks (Cmd-Z) before retrying.' The user has actionable info instead of a bare error. Also addresses cleanup - Added semantic-review/ to .gitignore so future review artifacts from the sub-agent don't accidentally land in commits. Build artifacts regenerated. Bundle grew from 60.20 kB to 62.25 kB JS (+0.6 kB gzip) — the new buildPlan refactor + validateMigrate pre-pass + post-numbering check. Acceptable for the correctness gain. What's still spec-only after this PR Items #4 and #6 in §0 status table are now ✅ shipped where they were partial; the remaining ❌ rows are unchanged (cross-page preflight, snapshot/rollback, undo ring buffer, i18n, reserved-name guard wired from inventory).
jackgranatowski
pushed a commit
that referenced
this pull request
Jun 2, 2026
- Fix #1: use normalizedQuery (trimmed) in template search branch so whitespace-only input doesn't switch to the full 275-token view - Fix #2: return applyToColorInput() result from onPickValue callback; show error toast and keep picker open if application fails - Fix #3: simplify toggleFamily() to always reset alphaOpen to a new Set(), preventing stale open-alpha state when switching families - Fix #4: reorder getBricksPanelRight() selectors — panel-specific IDs first, drop broad builder-root selectors (#brx-builder, .brx-builder) - Fix #5: add :focus-visible rules for all new interactive elements (.slashed-cp__qu-cell, .slashed-cp__scan-row, .slashed-cp__fam-banner, .slashed-cp__strip-sw, .slashed-cp__alpha-btn) - Fix #6: change .slashed-cp__strip-sw border from 0 to 1px solid transparent so --alpha dashed override actually renders https://claude.ai/code/session_01HesqASnoEx3Amm4H7f17jj
23 tasks
9 tasks
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.
Summary
This PR adds comprehensive light theme support to the design system and introduces a new theme application layer that properly applies color-scheme and base styling to themed elements.
Key Changes
Added light theme variables (
[data-theme="light"]) with a complete color palette mirroring the dark theme structure, including:Added
--sf-color-schemeCSS variable to both dark and light themes to enable propercolor-schemeresolutionCreated new
core/themes.csslayer that:color-schemeproperty to all[data-theme]elementsbackground-colorandcolorfrom theme variableslight-dark()CSS function resolution within themed subtrees (not just at root)Updated bundle configuration to include the new
core/themes.cssin the correct layer order (after states, before accessibility)Implementation Details
The new themes layer ensures that when a
[data-theme]attribute is applied to any element, the entire subtree respects that theme's color-scheme, allowing nested theme overrides and proper CSS function resolution throughout the component tree.https://claude.ai/code/session_01UJGjZdau27VkRjc35wNNdd
Summary by CodeRabbit