Sync viewport previews, revamp live preview, icon-trigger class hints - #229
Conversation
Svelte admin panel: - Live Scale Preview sliders in the Spacing and Typography tabs now scrub the actual Fluid Scale Viewport Range (viewport_min/viewport_max from the Spacing tab) instead of hardcoded bounds. The range is exposed via a shared viewportRangePx() helper so both previews — and the generated clamp() — share one source of truth; the Typography tab references the Spacing tab as that source. - Rewrote the bottom Live Preview into a token-driven design-set showcase: computes auto-contrasting on-color text, surfaces, text hierarchy, borders, links and status tints mirroring core/tokens.css. Fixes the unreadable white-on-light-status text and adds surfaces, typography, feedback alerts, buttons, a card and a form field. - Header height / sticky offset inputs now show the framework default values (3.5rem / 5rem) as placeholders. Bricks editor: - Class hints no longer trigger on hovering the whole class row (which obscured controls and was buggy). Instead a small "?" info icon is injected beside each SLASHED class row's action icons, and the tooltip is bound to that icon only. Reconcile-based, idempotent, fail-silent.
|
Warning Review limit reached
More reviews will be available in 39 minutes and 37 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR refactors the Bricks admin UI and editor interactions: a new viewport range helper consolidates hardcoded viewport constants used by preview components; the LivePreview component is completely redesigned to render token-driven design-system components; class hints transition from hover-based tooltips to injected icon buttons with focused event handling; and layout/typography hint text and default values are clarified and aligned. ChangesAdmin UI and Editor Enhancements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
plugins/SLASHED-for-WP/integrations/bricks/admin-app/src/components/LivePreview.svelte (2)
186-186: 💤 Low valueMinor: inconsistent variable name in sample text.
The preview text references
--sf-color-text--secondary(double hyphen) but the actual CSS variable used throughout is--c-text-secondary. Consider updating to match the preview's actual token name for clarity.🤖 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/admin-app/src/components/LivePreview.svelte` at line 186, The sample preview text in LivePreview.svelte uses the wrong CSS variable name `--sf-color-text--secondary`; replace that string with the actual token name `--c-text-secondary` so the preview matches the CSS variables used in the app (update the text/rendered sample that currently references `--sf-color-text--secondary` to `--c-text-secondary`).
94-99: Update CSSsign()browser support inonColor()compatibility note.
LivePreview.svelte(lines 94-99) uses CSSsign()inonColor().sign()is supported in Chrome/Edge 138+, Firefox 118+, and Safari 15.4+ (Baseline Newly Available: 2025-06-26). If the WordPress admin audience includes browsers older than these, add a fallback or document a supported-browser minimum.🤖 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/admin-app/src/components/LivePreview.svelte` around lines 94 - 99, The CSS sign() call in onColor() can be unsupported in some browsers; change onColor to use a CSS custom property fallback (e.g. replace sign(${TH} - l) with var(--sign-${name}, 1)) and in the same code where pairs are built set a per-color CSS var (--sign-<name>) via JS when rendering LivePreview.svelte: compute the sign from (TH - l) in JS and push a fallback declaration like `--sign-${name}: ${signValue}` into pairs (or set it on the preview element style) so older browsers use the JS-provided sign while modern browsers still resolve the real sign() expression.
🤖 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
`@plugins/SLASHED-for-WP/integrations/bricks/admin-app/src/components/LayoutsTab.svelte`:
- Line 88: Replace logical-OR fallbacks with nullish coalescing for the default
bindings so only null/undefined trigger the hardcoded fallback: change
expressions like defaults.header_height_mobile || '3.5' to use ??, and do the
same for the other affected bindings (the occurrences referencing
defaults.header_height_mobile, defaults.header_height_desktop and the other
header height/default fields at the other noted locations). Ensure each Svelte
input/default uses the ?? operator so legitimate falsy values (0 or empty
string) from defaults are preserved.
In
`@plugins/SLASHED-for-WP/integrations/bricks/editor-app/src/lib/class-hints.js`:
- Around line 316-333: The observer filter (touchesContainers) currently ignores
removed nodes so a detached anchored row can leave _activeBtn set while the
tooltip stays visible; update touchesContainers to also iterate m.removedNodes
and return true if any removed node contains or is the current _activeBtn (or
matches BRICKS_CONTAINERS/BUTTON selector) so reconcile() runs and clears
_activeBtn; reference the function touchesContainers, the variable _activeBtn,
the reconcile() caller, and BTN_CLASS/BRICKS_CONTAINERS selectors, and apply the
same removed-nodes check to the other similar observer/filter block (the one
around the 364–366 area).
---
Nitpick comments:
In
`@plugins/SLASHED-for-WP/integrations/bricks/admin-app/src/components/LivePreview.svelte`:
- Line 186: The sample preview text in LivePreview.svelte uses the wrong CSS
variable name `--sf-color-text--secondary`; replace that string with the actual
token name `--c-text-secondary` so the preview matches the CSS variables used in
the app (update the text/rendered sample that currently references
`--sf-color-text--secondary` to `--c-text-secondary`).
- Around line 94-99: The CSS sign() call in onColor() can be unsupported in some
browsers; change onColor to use a CSS custom property fallback (e.g. replace
sign(${TH} - l) with var(--sign-${name}, 1)) and in the same code where pairs
are built set a per-color CSS var (--sign-<name>) via JS when rendering
LivePreview.svelte: compute the sign from (TH - l) in JS and push a fallback
declaration like `--sign-${name}: ${signValue}` into pairs (or set it on the
preview element style) so older browsers use the JS-provided sign while modern
browsers still resolve the real sign() expression.
🪄 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: 521ccd06-7d41-4de5-8292-1f8dd82fa082
📒 Files selected for processing (13)
docs/roadmap.mdplugins/SLASHED-for-WP/integrations/bricks/admin-app/src/components/LayoutsTab.svelteplugins/SLASHED-for-WP/integrations/bricks/admin-app/src/components/LivePreview.svelteplugins/SLASHED-for-WP/integrations/bricks/admin-app/src/components/SpacingPreview.svelteplugins/SLASHED-for-WP/integrations/bricks/admin-app/src/components/TypographyPreview.svelteplugins/SLASHED-for-WP/integrations/bricks/admin-app/src/components/TypographyTab.svelteplugins/SLASHED-for-WP/integrations/bricks/admin-app/src/lib/stores.svelte.jsplugins/SLASHED-for-WP/integrations/bricks/assets/admin-app/app.cssplugins/SLASHED-for-WP/integrations/bricks/assets/admin-app/app.jsplugins/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/class-hints.jsplugins/SLASHED-for-WP/integrations/bricks/editor-app/src/styles/panel.css
| min={0} | ||
| step={0.25} | ||
| default={defaults.header_height_mobile ?? ''} | ||
| default={defaults.header_height_mobile || '3.5'} |
There was a problem hiding this comment.
Use nullish coalescing (??) instead of logical OR (||) for fallback defaults.
The || operator treats all falsy values (0, '', false, null, undefined) as triggers for the fallback, which could mask legitimate PHP defaults. If defaults.header_height_mobile or similar fields are explicitly set to 0 or an empty string in PHP, the operator will incorrectly fall back to the hardcoded '3.5' or '5' instead of using the actual PHP default.
Use the nullish coalescing operator (??) to only fall back when the value is strictly null or undefined.
🔧 Proposed fix
- default={defaults.header_height_mobile || '3.5'}
+ default={defaults.header_height_mobile ?? '3.5'}
- default={defaults.header_height_desktop || '5'}
+ default={defaults.header_height_desktop ?? '5'}
- default={defaults.sticky_offset_mobile || '3.5'}
+ default={defaults.sticky_offset_mobile ?? '3.5'}
- default={defaults.sticky_offset_desktop || '5'}
+ default={defaults.sticky_offset_desktop ?? '5'}Also applies to: 98-98, 113-113, 123-123
🤖 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/admin-app/src/components/LayoutsTab.svelte`
at line 88, Replace logical-OR fallbacks with nullish coalescing for the default
bindings so only null/undefined trigger the hardcoded fallback: change
expressions like defaults.header_height_mobile || '3.5' to use ??, and do the
same for the other affected bindings (the occurrences referencing
defaults.header_height_mobile, defaults.header_height_desktop and the other
header height/default fields at the other noted locations). Ensure each Svelte
input/default uses the ?? operator so legitimate falsy values (0 or empty
string) from defaults are preserved.
There was a problem hiding this comment.
Keeping || here intentionally. The default prop is used only as the input placeholder, and the PHP defaults for these four fields are empty strings ('header_height_mobile' => '', etc. in class-token-defaults.php) — that empty value means "no override, fall back to the framework value". Switching to ?? would yield '' ?? '3.5' → '', which re-introduces the very missing-placeholder bug this change fixes. With ||, an empty PHP default correctly surfaces the framework value (3.5rem / 5rem) as the placeholder. meta.defaults is the read-only framework default map, so a legitimate 0 is never expected for these height fields.
Generated by Claude Code
There was a problem hiding this comment.
@jackgranatowski, thanks for the clear explanation — that makes complete sense. Since class-token-defaults.php uses '' as the "no override" sentinel for these height fields, || is exactly right: it correctly promotes an empty-string PHP default to the visible framework placeholder (3.5 / 5), whereas ?? would pass the empty string through and silently swallow the placeholder. I'll withdraw the suggestion.
✏️ Learnings added
Learnt from: jackgranatowski
Repo: codeslash-dev/SLASHED PR: 229
File: plugins/SLASHED-for-WP/integrations/bricks/admin-app/src/components/LayoutsTab.svelte:88-88
Timestamp: 2026-06-05T00:31:10.366Z
Learning: In `plugins/SLASHED-for-WP/integrations/bricks/admin-app/src/components/LayoutsTab.svelte`, the `default` prop on `NumberField` for `header_height_mobile`, `header_height_desktop`, `sticky_offset_mobile`, and `sticky_offset_desktop` intentionally uses logical-OR (`||`) rather than nullish coalescing (`??`). The PHP defaults in `class-token-defaults.php` for these four fields are empty strings (`''`), which means "no override — show framework default as placeholder". `||` correctly promotes `''` to the hardcoded framework placeholder (`'3.5'` / `'5'`), whereas `??` would pass the empty string through unchanged. A legitimate value of `0` is never expected for these rem-based height fields.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: codeslash-dev/SLASHED PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-05-12T22:15:57.949Z
Learning: Applies to **/*.css : Reach for fluid tokens (space, text, leading with `clamp()`) first for responsive design, then container-aware primitives (`.grid`, `.stack`, `.cluster`), then breakpoint utilities as a last resort.
If a class row holding the active "?" icon is deleted while its tooltip is open, _activeBtn detaches but the fixed-position tooltip would stay visible pointing at nothing. The body observer now drops it as soon as the anchor leaves the DOM. Addresses CodeRabbit review on PR #229.
2 participants
Svelte admin panel:
the actual Fluid Scale Viewport Range (viewport_min/viewport_max from
the Spacing tab) instead of hardcoded bounds. The range is exposed via
a shared viewportRangePx() helper so both previews — and the generated
clamp() — share one source of truth; the Typography tab references the
Spacing tab as that source.
computes auto-contrasting on-color text, surfaces, text hierarchy,
borders, links and status tints mirroring core/tokens.css. Fixes the
unreadable white-on-light-status text and adds surfaces, typography,
feedback alerts, buttons, a card and a form field.
values (3.5rem / 5rem) as placeholders.
Bricks editor:
obscured controls and was buggy). Instead a small "?" info icon is
injected beside each SLASHED class row's action icons, and the tooltip
is bound to that icon only. Reconcile-based, idempotent, fail-silent.
Summary by CodeRabbit
New Features
Improvements
Documentation