Skip to content

Move spacing generator into Spacing Studio and add panels - #411

Closed
jackgranatowski wants to merge 1 commit into
mainfrom
codex/add-spacing-panels-to-spacingstudio
Closed

Move spacing generator into Spacing Studio and add panels#411
jackgranatowski wants to merge 1 commit into
mainfrom
codex/add-spacing-panels-to-spacingstudio

Conversation

@jackgranatowski

Copy link
Copy Markdown
Contributor

Motivation

  • Reorganize spacing controls into a purpose-built Spacing Studio to group global scale, rhythm, section and component spacing, and advanced engine controls for clearer discovery and editing.
  • Surface the most relevant spacing tokens per panel (Scale, Rhythm, Sections, Components, Advanced) so designers can find and tune related controls together.
  • Prevent duplicate generator UI by moving the space scale generator into the Studio and stopping it from rendering separately in the domain panel.

Description

  • Add five panels (Scale, Rhythm, Sections, Components, Advanced) and a tabbed navigation to configurator/src/components/editors/SpacingStudio.svelte, plus per-panel descriptions and curated token groups.
  • Move the ScaleGenerator (space generator) into the Scale panel inside SpacingStudio.svelte so the generator is shown only within the Studio UI via <ScaleGenerator kinds={['space']} />.
  • Update Studio token lists so Rhythm exposes --sf-space-scale, --sf-gap, --sf-content-gap, and --sf-flow-space; Sections exposes --sf-section-* tokens; Components exposes --sf-component-pad and common component spacing tokens; Advanced exposes the fluid engine source tokens and composition gaps.
  • Stop rendering the spacing generator outside the Studio by changing the generator condition in configurator/src/components/DomainPanel.svelte to exclude domain.id === 'spacing', and add small CSS/markup tweaks (workflow buttons, panel header, and use of --sf-flow-space in rhythm preview).

Testing

  • Ran npm run check (Svelte diagnostics) which completed with 0 errors and 2 warnings.
  • Ran npm run build (Vite production build) which completed successfully and produced the distributable bundles.
  • Attempted an automated Playwright screenshot to validate the running UI, but it was blocked because the Chromium browser executable is not installed in the environment.

Codex Task

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@jackgranatowski, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 28 minutes and 50 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2fc87eed-23aa-4ed4-9081-f67a8a1d8e18

📥 Commits

Reviewing files that changed from the base of the PR and between f391ec2 and 6c57f0a.

📒 Files selected for processing (2)
  • configurator/src/components/DomainPanel.svelte
  • configurator/src/components/editors/SpacingStudio.svelte
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/add-spacing-panels-to-spacingstudio

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.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Spacing Studio: add tabbed panels and move space generator inside
✨ Enhancement 🐞 Bug fix 🕐 40+ Minutes

Grey Divider

Description

• Add five Spacing Studio panels with tab navigation and curated token groupings.
• Render the space ScaleGenerator only inside the Studio (Scale panel) to avoid duplicate UI.
• Polish spacing previews/styles (panel header, workflow buttons, rhythm preview uses
 --sf-flow-space).
Diagram

graph TD
  A["DomainPanel.svelte"] --> B["SpacingStudio.svelte"] --> C["Active panel state"] --> D["StudioControls"] --> E["resolveStudioGroups()"] --> F["tokenByName"]
  C --> G["ScaleGenerator"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Move panel definitions into lib/studioSchema.js
  • ➕ Keeps studio panel/group configuration centralized and easier to reuse/test
  • ➕ Avoids duplicating token lists in Svelte components as more Studios gain panels
  • ➖ Adds indirection for a spacing-only UI; may slow iteration while the panel model is still evolving
2. Split each panel into its own Svelte component
  • ➕ Keeps SpacingStudio.svelte smaller and isolates panel-specific preview/controls logic
  • ➕ Easier targeted UI testing and future per-panel enhancements
  • ➖ More files/components to maintain; overhead may not pay off if panels stay simple
3. Gate generator rendering by Studio presence (usesVisualStudio) instead of domain.id
  • ➕ More generic rule: domains with a Studio own their generator placement
  • ➕ Avoids special-casing the spacing domain id
  • ➖ Could unintentionally suppress generators for other domains if they later add Studios but still expect inline generators

Recommendation: Current approach is reasonable: it cleanly eliminates duplicate generator UI and makes spacing controls easier to discover via panels. If additional Studios adopt similar panel navigation, consider extracting the panel config to studioSchema.js (or splitting panels into components) to keep token lists and descriptions maintainable.

Files changed (2) +79 / -11

Enhancement (1) +78 / -10
SpacingStudio.svelteAdd panel navigation and host the space ScaleGenerator in the Scale panel +78/-10

Add panel navigation and host the space ScaleGenerator in the Scale panel

• Replaces the prior fixed workflow/groups setup with a five-panel model (Scale, Rhythm, Sections, Components, Advanced), each with descriptions and curated token groups resolved via 'resolveStudioGroups'. Mounts '<ScaleGenerator kinds={['space']} collapsible />' only when the Scale panel is active, and updates layout/styles (button workflow nav, panel header, and rhythm preview gap using '--sf-flow-space').

configurator/src/components/editors/SpacingStudio.svelte

Bug fix (1) +1 / -1
DomainPanel.svelteSuppress spacing-domain generators in the domain panel +1/-1

Suppress spacing-domain generators in the domain panel

• Tightens the 'hasGenerators' condition to exclude 'domain.id === 'spacing'', preventing the space scale generator from rendering outside Spacing Studio. This removes duplicate generator UI while preserving generator behavior for other domains (e.g., typography).

configurator/src/components/DomainPanel.svelte

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 5 rules

Grey Divider


Informational

1. Stale generator-domain comment 🐞 Bug ⚙ Maintainability
Description
DomainPanel.svelte still documents spacing as a generator domain, but hasGenerators now explicitly
excludes domain.id === 'spacing', making the comment misleading for future preview/layout changes.
This is documentation drift introduced by the change and can cause incorrect assumptions during
later refactors.
Code

configurator/src/components/DomainPanel.svelte[R97-99]

  // Domains with a scale generator (typography, spacing): preview goes BELOW
  // the generator so the specimen updates right next to the controls.
-  const hasGenerators = $derived(generators.length > 0);
+  const hasGenerators = $derived(generators.length > 0 && domain.id !== 'spacing');
Relevance

⭐⭐⭐ High

Team often fixes documentation/metadata drift; e.g., accepted taxonomy/description correction in PR
#339.

PR-#339

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The comment claims spacing is handled as a generator-domain, but the updated hasGenerators logic
excludes spacing; generator rendering is gated by hasGenerators, so spacing will not render
generators in DomainPanel anymore.

configurator/src/components/DomainPanel.svelte[97-100]
configurator/src/components/DomainPanel.svelte[195-205]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`DomainPanel.svelte`’s comment still states that both typography and spacing are “generator domains”, but the new `hasGenerators` logic explicitly excludes spacing. This mismatch can mislead future maintainers when changing generator/preview layout logic.

### Issue Context
Spacing’s generator is now intended to live inside `SpacingStudio`, so `DomainPanel` should no longer describe spacing as using the generator-domain layout path.

### Fix Focus Areas
- configurator/src/components/DomainPanel.svelte[97-99]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Duplicated spacing group definitions 🐞 Bug ⚙ Maintainability
Description
SpacingStudio.svelte now hardcodes its own spacing panel/group token lists while studioSchema.js
still defines STUDIO_GROUPS.spacing, creating two overlapping sources of truth. This increases the
risk of future drift (updates applied in one place but not the other).
Code

configurator/src/components/editors/SpacingStudio.svelte[R1-5]

<script>
-  import { STUDIO_GROUPS, resolveStudioGroups } from '../../lib/studioSchema.js';
+  import { resolveStudioGroups } from '../../lib/studioSchema.js';
  import StudioFrame from './StudioFrame.svelte';
  import StudioControls from './StudioControls.svelte';
+  import ScaleGenerator from '../ScaleGenerator.svelte';
Relevance

⭐ Low

Similar “avoid hardcoded lists / single source of truth” suggestion was rejected; team tolerates
curated inline token groupings.

PR-#369

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
SpacingStudio now defines spacing groups inline (via panels[].groups), while studioSchema.js
still exports a spacing grouping definition that is no longer used by SpacingStudio. This
duplication is a maintainability hazard.

configurator/src/components/editors/SpacingStudio.svelte[1-45]
configurator/src/lib/studioSchema.js[36-41]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`SpacingStudio.svelte` defines spacing groups/panels locally, but `STUDIO_GROUPS.spacing` still exists in `studioSchema.js`. Maintaining both increases the chance of future inconsistencies.

### Issue Context
Other studios (layout/borders/shadows/motion/effects) still use `STUDIO_GROUPS.*`, while spacing has moved to a local `panels` definition.

### Fix Focus Areas
- configurator/src/components/editors/SpacingStudio.svelte[1-45]
- configurator/src/lib/studioSchema.js[36-41]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant