Add UI composition size tokens - #335345
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
The spacing validator omits 0px and conflicts with canonical authoring guidance.
Pull request overview
Adds behavior-neutral icon-size tokens and expands the spacing-token foundation for predictable UI composition.
Changes:
- Registers icon sizes from 12px to 32px.
- Adds 1px and 3px spacing tokens.
- Updates known-variable registration, validation, and focused tests.
File summaries
| File | Changes and findings |
|---|---|
src/vs/platform/theme/test/common/sizeRegistry.test.ts |
Tests new token registration and values. |
src/vs/platform/theme/common/sizes/baseSizes.ts |
Registers icon-size and spacing tokens. |
build/lib/test/designTokens.test.ts |
Tests spacing validation behavior. |
build/lib/stylelint/vscode-known-variables.json |
Registers generated CSS variables. |
build/lib/stylelint/validateDesignTokens.ts |
Updates the spacing ramp. Moderate (1 vote): include 0px in SPACING_SCALE. Nit (1 vote): align authoring guidance with the expanded ramp. |
Review details
Suppressed comments (2)
build/lib/stylelint/validateDesignTokens.ts:357
- The registered ramp includes
spacing.sizeNone(0px), but 0 is missing fromSPACING_SCALE. BecausesnapSpacinguses only this array, an off-scale value such as0.25pxis suggested as1pxinstead of the nearest ramp value0px; include 0 so snapping covers the complete ramp.
const SPACING_SCALE: readonly number[] = [1, 2, 3, 4, 6, 8, 10, 12, 16, 20, 24, 28, 32, 36, 40];
build/lib/stylelint/validateDesignTokens.ts:357
- This makes 1px and 3px on-scale, but the canonical authoring guidance still defines the ramp as 0, 2, 4, … and explicitly says to snap 1px to 2px and 3px to 4px (
.github/instructions/design-tokens.instructions.md:18-41). That leaves the validator and the guidance giving contradictory answers to CSS authors; update the guidance (and mirrored references) as part of this rollout or defer the validator change until it lands.
const SPACING_SCALE: readonly number[] = [1, 2, 3, 4, 6, 8, 10, 12, 16, 20, 24, 28, 32, 36, 40];
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The updated spacing validator conflicts with the repository’s canonical authoring guidance.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Nearest-step validation currently excludes zero from the declared spacing ramp.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
|
Fixed the nearest-step zero-scale bug in 7c5a6f6: Rebased #335346 and #335350 on top (no content changes there, new heads only). Re-requesting Copilot review. |
There was a problem hiding this comment.
🔵 Needs a closer look
The implementation is coherent, but adopting a global candidate vocabulary whose roles remain unvalidated requires human design review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
7c5a6f6 to
c435159
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Icon token descriptions currently present unvalidated contextual roles as settled guidance.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The implementation is mechanically consistent, but landing the candidate vocabulary requires the human design-system judgment explicitly requested by the proposal.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the change-oriented composition token assertion with durable, family-level coverage for the complete spacing and icon size ramps. Keep the existing curated registration smoke test unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep the existing representative registration smoke test unchanged. Discover spacing and icon size families by registry prefix so each complete expected scale is declared only once. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Compare complete registry families as keyed default-value snapshots so the tests cover membership and values without asserting declaration order. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
validateSpacingTokens declared its nearest-step scale starting at 1px, so subpixel values close to zero (e.g. 0.4px) could never snap to sizeNone: the smallest candidate was always 1px. Add 0 to SPACING_SCALE so near-zero values resolve to var(--vscode-spacing-sizeNone), while the existing tie-break (<=) still rounds an exact 0.5px tie up to 1px as documented. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
762ee7d to
5bafbc2
Compare
Cherry Wang (chryw)
changed the title
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cherry Wang (chryw)
changed the title
Predictable UI composition — 1 of 3: shared token foundation
Stack:
WorkbenchButtonBarpilotChange
Adds shared vocabulary for recurring UI geometry:
0, 2, 4, 6, ...scale;Token descriptions are deliberately neutral. The scale provides stable names for values already common in VS Code; individual components still decide which step fits their density and role.
Scope boundary
This PR changes global vocabulary and validation, not product geometry. It does not migrate components, assign settled product meanings to every step, or require raw on-scale values to be replaced mechanically.
The active #335350 pilot consumes the 16px icon box. Other steps remain available for separately reviewed product work without requiring each feature to invent another variable.
Validation
git diff --checkare clean.Decision requested
Do the recurring 1px/3px spacing values and 12/16/20/24/32 icon sizes form the right neutral shared vocabulary without prematurely prescribing product roles?