fix(bricks): skip color-palette injection on Bricks 2.2+ Color Manager - #182
Conversation
Bricks 2.2's Color Manager materializes every color-palette entry into :root as a static value (plus a [data-brx-theme="dark"] variant), which overrides the framework's adaptive light-dark() --sf-color-* tokens and freezes dark/light switching. Because the plugin injects the palette and the dark_mode hex equals the light hex, the section bg renders the same color in both modes. On Bricks 2.2+ skip palette injection (slashed_bricks_supports_color_manager(), gated by the slashed_bricks/inject_color_palette filter); tokens remain available via the Variable Manager, which registers empty-value entries Bricks never writes to :root. Version is compared on major.minor so 2.2 pre-release builds are detected. Co-Authored-By: Jack Granatowski <jack.granatowski@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdded a version detection helper to identify Bricks 2.2+ Color Manager support, made palette injection conditional on that detection via a new guard method, and exposed a filter hook to override injection behavior. Documented the gating logic, filter usage, and architectural rationale in the README. ChangesColor Manager Support Detection and Conditional Palette Injection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/SLASHED-for-WP/integrations/bricks/slashed-bricks.php (1)
151-178: ⚡ Quick winVersion gating logic is correct; consider extracting the shared version-resolution block.
The
major.minorcomparison correctly catches pre-release2.2-*builds thatversion_compare()would rank below2.2. Edge cases check out (2.10→ true,2.1.5→ false,3.0→ true).The
BRICKS_VERSION/theme-metadata resolution in Lines 154-165 duplicates the same block inslashed_bricks_is_bricks_active()(Lines 119-131). Extracting a smallslashed_bricks_get_bricks_version()helper keeps both detectors in sync if Bricks ever changes how its version is exposed.♻️ Proposed extraction
/** * Resolve the active Bricks version string, or '' if unknown. * * `@return` string */ function slashed_bricks_get_bricks_version() { if ( defined( 'BRICKS_VERSION' ) ) { return (string) BRICKS_VERSION; } $theme = wp_get_theme(); if ( 'bricks' === strtolower( $theme->get_template() ) ) { $parent = $theme->parent(); return $parent ? (string) $parent->get( 'Version' ) : (string) $theme->get( 'Version' ); } if ( 'bricks' === strtolower( $theme->get( 'Name' ) ) ) { return (string) $theme->get( 'Version' ); } return ''; }Then
slashed_bricks_supports_color_manager()becomes:function slashed_bricks_supports_color_manager() { - $version = ''; - - if ( defined( 'BRICKS_VERSION' ) ) { - $version = (string) BRICKS_VERSION; - } else { - $theme = wp_get_theme(); - - if ( 'bricks' === strtolower( $theme->get_template() ) ) { - $parent = $theme->parent(); - $version = $parent ? (string) $parent->get( 'Version' ) : (string) $theme->get( 'Version' ); - } elseif ( 'bricks' === strtolower( $theme->get( 'Name' ) ) ) { - $version = (string) $theme->get( 'Version' ); - } - } - + $version = slashed_bricks_get_bricks_version(); if ( '' === $version || ! preg_match( '/^(\d+)\.(\d+)/', $version, $m ) ) { return false; }🤖 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 `@plugins/SLASHED-for-WP/integrations/bricks/slashed-bricks.php` around lines 151 - 178, Extract the duplicated BRICKS_VERSION/theme metadata resolution into a new helper slashed_bricks_get_bricks_version() that returns the Bricks version string or '' and replace the inline resolution in both slashed_bricks_supports_color_manager() and slashed_bricks_is_bricks_active() with calls to that helper; ensure the helper implements the same logic currently used (check defined('BRICKS_VERSION'), then wp_get_theme() and compare strtolower($theme->get_template()) and strtolower($theme->get('Name')) and return parent or theme Version as appropriate) so both callers get the same canonical version string for the existing major/minor comparison.
🤖 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.
Nitpick comments:
In `@plugins/SLASHED-for-WP/integrations/bricks/slashed-bricks.php`:
- Around line 151-178: Extract the duplicated BRICKS_VERSION/theme metadata
resolution into a new helper slashed_bricks_get_bricks_version() that returns
the Bricks version string or '' and replace the inline resolution in both
slashed_bricks_supports_color_manager() and slashed_bricks_is_bricks_active()
with calls to that helper; ensure the helper implements the same logic currently
used (check defined('BRICKS_VERSION'), then wp_get_theme() and compare
strtolower($theme->get_template()) and strtolower($theme->get('Name')) and
return parent or theme Version as appropriate) so both callers get the same
canonical version string for the existing major/minor comparison.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 98adf7f4-fb58-4e8a-bafb-1d27ef3808f6
📒 Files selected for processing (3)
plugins/SLASHED-for-WP/integrations/bricks/README.mdplugins/SLASHED-for-WP/integrations/bricks/includes/class-colors.phpplugins/SLASHED-for-WP/integrations/bricks/slashed-bricks.php
Dedupe the BRICKS_VERSION/theme-metadata version resolution shared by slashed_bricks_is_bricks_active() and slashed_bricks_supports_color_manager() so both detectors stay in sync. Behavior unchanged. (CodeRabbit nitpick on #182.) Co-Authored-By: Jack Granatowski <jack.granatowski@gmail.com>
|
Thanks @coderabbitai — applied the nitpick in 8712d88: extracted |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh api repos/codeslash-dev/SLASHED/commits/8712d88 --jq '.files[] | {filename: .filename, patch: .patch}'Length of output: 3075
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
1 participant
Summary
Dark/light mode silently breaks for SLASHED colors in Bricks 2.2+. Bricks 2.2's new Color Manager materializes every color-palette entry into
:rootas a static value (plus a[data-brx-theme="dark"]variant). When the plugin injects the SLASHED palette, Bricks bakes each token to fixed hex on:root, which overrides the framework's adaptivelight-dark()--sf-color-*definitions. Worse, the injecteddark_modehex equals the light hex (added in50dcfeb), so the dark:rootblock is identical to light — the toggle flipscolor-scheme/data-brx-themebutvar(--sf-color-primary)never changes.Observed in a user export (unlayered, so it beats every
@layer):Fix: on Bricks 2.2+, don't inject SLASHED palettes into
bricks_color_palette. Tokens stay reachable via the Variable Manager (class-variables.php), whose empty-value entries Bricks never writes to:root, so the framework remains the single source of truth andlight-dark()drives theming again. Pre-2.2 behavior is unchanged.Changes
slashed-bricks.php: newslashed_bricks_supports_color_manager(). Resolves the Bricks version (BRICKS_VERSION, else theme version) and compares on major.minor (>= 2.2) —version_compare()ranks2.2-betabelow2.2, which would miss the beta where the Color Manager first shipped.class-colors.php:inject_palettes()now early-returns (after the usual strip) whenshould_inject_palettes()is false. Default =! supports_color_manager, overridable via the newslashed_bricks/inject_color_palettefilter (__return_trueto force injection on 2.2+, accepting static hex snapshots without dark-mode adaptation). The version check runs inside the filter (not the constructor) becauseBRICKS_VERSIONisn't defined yet atplugins_loaded, where the managers are instantiated.strip_palettesstill runs on save to clean any stale rows.README.md: documents the 2.2 behavior + the new filter.Testing
php -lclean on both changed files.1.9.2/2.1.5→ inject;2.2,2.2.0,2.2-beta,2.2.1-RC1,2.3.1,2.10,3.0→ skip; empty/garbage → inject (safe pre-2.2 default);slashed_bricks/inject_color_paletteoverride → inject. All pass.:rootshould no longer hardcode--sf-color-primary, and its computed value should be anoklch(...)that differs betweendata-brx-theme="light"and"dark".Follow-up
Restoring color swatches in the 2.2 Color Manager without breaking adaptivity (e.g. injecting real distinct light/dark hex, or a Color-Manager-native registration) is left as a QoL follow-up; the Variable picker already previews tokens on the canvas.
Link to Devin session: https://app.devin.ai/sessions/113c6a747fc6406c9510e0e88892441f
Requested by: @jackgranatowski
Summary by CodeRabbit
Release Notes
Documentation
New Features