Skip to content

revert(bricks): remove SF colour border/box-shadow fixes - #212

Merged
jackgranatowski merged 1 commit into
mainfrom
claude/bricks-border-sf-color-bug-f33Qj
Jun 3, 2026
Merged

revert(bricks): remove SF colour border/box-shadow fixes#212
jackgranatowski merged 1 commit into
mainfrom
claude/bricks-border-sf-color-bug-f33Qj

Conversation

@jackgranatowski

@jackgranatowski jackgranatowski commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Reverts the three SF colour border/box-shadow fix commits (93da924, cafa170, 18303c0) that were introduced to handle event-ordering issues with Bricks' click-outside handler
  • The mousedown-based approach stopped working after a Bricks Builder version update
  • Removes all related changes: data-cp-pick-var attribute on swatch buttons, direct addEventListener in onMount, suppress flag in pick(), and SF button mousedown handler in color-swatches.js
  • Rebuilds editor-app/assets/app.js to match the reverted source

Why

The fix relied on firing our mousedown handler at element level (before Bricks' document-level click-outside handler), but a Bricks update changed the event handling in a way that broke this approach. The border/box-shadow SF colour selection will be handled manually.

Test plan

  • SF colour panel opens normally for text and background controls
  • Selecting a colour from the SF panel applies it correctly for text/background
  • Border and box-shadow controls: SF colour panel opens; note that apply will show "input not found" (expected — this is the known limitation being reverted to)

Generated by Claude Code

Summary by CodeRabbit

  • Refactor
    • Streamlined color picker event handling and interaction flow for improved efficiency.
    • Optimized swatch button behavior implementation.
    • Simplified color input integration logic.

@coderabbitai

coderabbitai Bot commented Jun 3, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

ColorPanel picker interaction is refactored from per-button dataset suppression flags to root-level mousedown propagation stopping. Component-owned event binding and pick() suppression logic are removed. All swatch button data-cp-pick-var attributes are removed across panel sections and the ColorSwatch component. Integration button mousedown listener is removed.

Changes

Color picker event handling simplification

Layer / File(s) Summary
ColorPanel core event handler refactor
plugins/SLASHED-for-WP/integrations/bricks/editor-app/src/components/ColorPanel.svelte
Remove panelEl reference binding and onMount mousedown handler that supplied onPickValue calls with dataset suppression logic. Simplify pick() by removing the conditional branch that detected and cleared button suppression flags. Replace panel root onclick propagation stop with onmousedown propagation stop.
Remove data-cp-pick-var from swatch buttons
plugins/SLASHED-for-WP/integrations/bricks/editor-app/src/components/ColorPanel.svelte, plugins/SLASHED-for-WP/integrations/bricks/editor-app/src/components/ColorSwatch.svelte
Remove data-cp-pick-var attribute from all swatch buttons in ColorPanel (Quick Use, Brand/Status base/scale/alpha sections) and ColorSwatch component while preserving onclick handlers that invoke pick().
Integration button mousedown removal
plugins/SLASHED-for-WP/integrations/bricks/editor-app/src/lib/color-swatches.js
Remove mousedown event listener that stopped propagation and prevented default behavior from the injected SF button. Click and keydown handlers remain for panel interaction.

Possibly related PRs

  • codeslash-dev/SLASHED#201: Both PRs modify ColorPanel.svelte event handling to use root-level stopPropagation to prevent outside-close behavior.
  • codeslash-dev/SLASHED#208: Both PRs modify ColorPanel.svelte and ColorSwatch.svelte picker wiring by removing data-cp-pick-var and changing mousedown handling.
  • codeslash-dev/SLASHED#206: Both PRs modify ColorPanel.svelte and color-swatches.js mousedown propagation/preventDefault logic to manage Bricks popover focus.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'revert(bricks): remove SF colour border/box-shadow fixes' accurately describes the main change: reverting commits that implemented SF colour selection fixes and removing related code artifacts.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/bricks-border-sf-color-bug-f33Qj

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Reverts commits 93da924, cafa170, and 18303c0.

The mousedown-based approach to beat Bricks' click-outside handlers did not
work reliably after a Bricks update. Removing all three fix commits so the
border/box-shadow SF colour flow can be addressed separately.

https://claude.ai/code/session_016aL3vh5nHHa2QbmAxyFCGy
@jackgranatowski
jackgranatowski force-pushed the claude/bricks-border-sf-color-bug-f33Qj branch from 3507b00 to ed77906 Compare June 3, 2026 21:25

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
plugins/SLASHED-for-WP/integrations/bricks/editor-app/src/components/ColorPanel.svelte (1)

202-203: 💤 Low value

Both onclick and onmousedown propagation stops are present.

The review stack context describes this change as "Switch panel root event handling from onclick propagation stop to onmousedown propagation stop", which suggests onclick should have been replaced. However, both handlers are now present on the panel root.

Functionally, this is safe and defensive—it stops propagation for both event types regardless of whether Bricks listens to click or mousedown events. If this dual-handler approach is intentional for maximum compatibility, consider briefly documenting it in a comment. Otherwise, if onclick is no longer needed, it could be removed to match the stated simplification intent.

🤖 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/components/ColorPanel.svelte`
around lines 202 - 203, The panel root in ColorPanel.svelte currently has both
onclick and onmousedown propagation stops but the change intent was to switch
from onclick to onmousedown; remove the redundant onclick={(e) =>
e.stopPropagation()} attribute so only onmousedown={(e) => e.stopPropagation()}
remains on the panel root (or, if you intended to keep both for compatibility,
replace removal with a one-line comment above the handlers explaining the
deliberate dual handling); locate these attributes in the ColorPanel.svelte
component and update accordingly.
🤖 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/components/ColorPanel.svelte`:
- Around line 202-203: The panel root in ColorPanel.svelte currently has both
onclick and onmousedown propagation stops but the change intent was to switch
from onclick to onmousedown; remove the redundant onclick={(e) =>
e.stopPropagation()} attribute so only onmousedown={(e) => e.stopPropagation()}
remains on the panel root (or, if you intended to keep both for compatibility,
replace removal with a one-line comment above the handlers explaining the
deliberate dual handling); locate these attributes in the ColorPanel.svelte
component and update accordingly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 89de1537-8cc2-4dad-8714-29af4cbad28e

📥 Commits

Reviewing files that changed from the base of the PR and between 9060438 and ed77906.

📒 Files selected for processing (4)
  • plugins/SLASHED-for-WP/integrations/bricks/assets/editor-app/app.js
  • plugins/SLASHED-for-WP/integrations/bricks/editor-app/src/components/ColorPanel.svelte
  • plugins/SLASHED-for-WP/integrations/bricks/editor-app/src/components/ColorSwatch.svelte
  • plugins/SLASHED-for-WP/integrations/bricks/editor-app/src/lib/color-swatches.js
💤 Files with no reviewable changes (2)
  • plugins/SLASHED-for-WP/integrations/bricks/editor-app/src/components/ColorSwatch.svelte
  • plugins/SLASHED-for-WP/integrations/bricks/editor-app/src/lib/color-swatches.js

@jackgranatowski
jackgranatowski merged commit 422603a into main Jun 3, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants