feat(bricks): add color swatches to variable-picker dropdown - #184
Conversation
Paint a colour square next to each --sf-color-* entry in the Bricks variable-picker dropdown, builder-side only. Variables stay empty-valued so nothing hits :root and dark/light remains framework-driven; the swatch is rendered from the server-resolved hex map (get_color_hex_map), localised alongside the existing class-hints config. Additive and fail-silent, reusing the class-hints decoration pattern: a debounced MutationObserver reconciles each row every pass (Bricks/Vue reuses <li> nodes when the search filter runs, so a one-time marker would go stale). resolveSwatchColor() is pure/DOM-free and only matches known --sf-color-* tokens, so the same observer is harmless in the spacing / typography pickers. Restores the swatch affordance lost when the Color Palette injection was disabled on Bricks 2.2+ (#182). Toggle via slashed_bricks/show_color_swatches. Refs #183 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:
📝 WalkthroughWalkthroughThis PR introduces an optional Bricks builder feature that displays color swatches next to ChangesBricks Builder Color Swatches
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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/editor-app/src/lib/color-swatches.js (1)
186-187: 💤 Low valueConsider the performance impact of observing
document.body.Observing
document.bodywith{ childList: true, subtree: true }will trigger on every DOM mutation in the page, not just the variable picker. While the debouncedschedule()mitigates excessive reconciliation passes, you might consider narrowing the observation scope if the picker is mounted in a known container.However, the comment on lines 181-185 explains that the picker is "created, destroyed, and re-rendered" by Bricks dynamically, which may justify the broad scope. If the picker's container is predictable, you could observe that instead of body.
🤖 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/editor-app/src/lib/color-swatches.js` around lines 186 - 187, The code currently creates a MutationObserver named _observer and calls _observer.observe(document.body, { childList: true, subtree: true }), which can be expensive; change the observer target to a narrower container when possible by finding the picker's mount element (e.g., the Bricks picker container or its parent) instead of document.body, and fall back to document.body only if the picker container cannot be determined. Update the logic around _observer and the existing schedule() usage so observe() is invoked on that container element (or document.body fallback) and ensure disconnect/reconnect behavior still uses _observer to avoid leaking observers when the picker is destroyed and recreated.
🤖 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/editor-app/src/lib/color-swatches.js`:
- Around line 186-187: The code currently creates a MutationObserver named
_observer and calls _observer.observe(document.body, { childList: true, subtree:
true }), which can be expensive; change the observer target to a narrower
container when possible by finding the picker's mount element (e.g., the Bricks
picker container or its parent) instead of document.body, and fall back to
document.body only if the picker container cannot be determined. Update the
logic around _observer and the existing schedule() usage so observe() is invoked
on that container element (or document.body fallback) and ensure
disconnect/reconnect behavior still uses _observer to avoid leaking observers
when the picker is destroyed and recreated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 926845a3-c3f4-4fee-aaaa-42b5657f2242
📒 Files selected for processing (7)
plugins/SLASHED-for-WP/integrations/bricks/README.mdplugins/SLASHED-for-WP/integrations/bricks/assets/editor-app/app.cssplugins/SLASHED-for-WP/integrations/bricks/assets/editor-app/app.jsplugins/SLASHED-for-WP/integrations/bricks/editor-app/src/lib/color-swatches.jsplugins/SLASHED-for-WP/integrations/bricks/editor-app/src/main.jsplugins/SLASHED-for-WP/integrations/bricks/editor-app/src/styles/panel.cssplugins/SLASHED-for-WP/integrations/bricks/includes/class-rebemer-enqueue.php
Filter the body-level MutationObserver with touchesPicker() so only mutations inside the variable picker (a row patch or the dropdown subtree being added) schedule a reconcile pass. Unrelated builder churn (canvas edits, structure-panel rebuilds) no longer wakes the reconciler. Keeps the robust body scope (the picker has no container that's stable across its whole lifecycle) while removing the wasted passes CodeRabbit flagged. Co-Authored-By: Jack Granatowski <jack.granatowski@gmail.com>
|
@coderabbitai Addressed the observer-scope nitpick in 6a49975. I kept the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check the updated color-swatches.js after commit 6a49975
git show 6a49975 -- plugins/SLASHED-for-WP/integrations/bricks/editor-app/src/lib/color-swatches.js 2>/dev/null || \
cat plugins/SLASHED-for-WP/integrations/bricks/editor-app/src/lib/color-swatches.js 2>/dev/null || \
fd "color-swatches.js" --exec cat {}Length of output: 4181
What's working well:
One minor edge case to be aware of (non-blocking): If Bricks ever replaces the entire picker container (rather than adding rows into it), the The fix cleanly addresses the concern without sacrificing robustness. |
1 participant
Summary
Restores colour swatches in the Bricks builder without re-introducing the dark-mode bug that #182 fixed. Closes the follow-up tracked in #183.
Since #182 disabled Color-Palette injection on Bricks 2.2+ (the Color Manager bakes palette colours into
:rootas static hex, freezinglight-dark()),--sf-color-*tokens are now reached via the Variable Manager. Those variables are deliberately empty-valued so nothing hits:root— which also means Bricks has no value to draw a swatch from, so the picker shows plain text.This paints the swatch back in, builder-side only, the way CORE Framework does it: a small colour square is prepended to each
--sf-color-*row in the variable-picker dropdown. The colour comes from the existing server-resolved hex map (Slashed_Bricks_Inventory::get_color_hex_map()), localised alongside the existing class-hints config — the builder panel doesn't load the SLASHED stylesheet, sovar()can't resolve there. Nothing is written to:root; dark/light stays 100% framework-driven.How it hooks the picker
The dropdown markup (captured from a live 2.2 install) is stable and semantic:
A debounced
MutationObserverreconciles each row on every pass rather than stamping once: Bricks (Vue) reuses<li>nodes when the search box filters, so a row that wassf-color-actioncan becomesf-color-primary— or a non-colour variable — while keeping the same node. Each pass recomputes the desired colour and adds / updates / removes the swatch accordingly, which is reuse-safe where a one-time marker would go stale.Design guarantees
class-hints.jspattern (scoped, idempotent,AbortSignalteardown) in the same editor-app bundle.-a*tokens read as transparent at a glance.add_filter('slashed_bricks/show_color_swatches', '__return_false')— also skips localising the hex map when off.Changes
editor-app/src/lib/color-swatches.js(new) — observer +resolveSwatchColor()reconciler.editor-app/src/main.js— init instart(), teardown inbeforeunload.editor-app/src/styles/panel.css—.slashed-var-swatchstyling (checkerboard underlay).includes/class-rebemer-enqueue.php— localiseshowColorSwatches+colorHexMap; new filter.assets/editor-app/app.{js,css}— rebuilt bundle.README.md— feature + filter docs.Testing
php -lon the modified PHP — clean.npm run lint:css(repo root) — clean.npm run buildineditor-app/— builds; swatch code present in the committed bundle.--sf-color-*entry; selecting one and toggling dark/light still recolours the canvas; if the script is disabled/broken the dropdown still lists and selects every variable.Refs #183
Link to Devin session: https://app.devin.ai/sessions/113c6a747fc6406c9510e0e88892441f
Requested by: @jackgranatowski
Summary by CodeRabbit
New Features
Documentation