fix(pds-box): add core tokens to spacing set #565
Conversation
✅ Deploy Preview for pine-design-system ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
bot
added
bug
|
@coderabbitai please review this PR |
|
@QuintonJason: I'll review the PR changes now. ✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request renames the spacing type Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/core/src/utils/types.ts (1)
58-66: Type definition mismatch with documented API—type must be updated.The
BoxShadowSizeTypeinlibs/core/src/utils/types.tslines 58-66 defines only numeric tokens ('050', '100', etc.), but the codebase extensively uses semantic aliases ('xs', 'sm', 'md', 'lg') across documentation, stories, and tests:
libs/core/src/components/pds-box/docs/pds-box.mdx: shadow="xs|sm|md|lg"libs/core/src/components/pds-box/test/pds-box.spec.tsx: shadow="xs"libs/core/src/components/pds-row/stories/pds-row.stories.js: shadow="sm"libs/core/src/stories/guides/layout.docs.mdx: shadow="md|sm"Update
BoxShadowSizeTypeto include the semantic aliases or resolve to numeric tokens to match actual usage.
🧹 Nitpick comments (4)
libs/core/src/components/pds-row/readme.md (1)
19-19: Doc wording nit: tighten phrasing for minHeight.Suggest “Works with alignment props.” instead of “Used in conjunction with alignment props.”
libs/core/src/components/pds-box/docs/pds-box.mdx (1)
20-21: List all supported justify-content values for clarity.Add “space-evenly” explicitly (it’s supported on Box), instead of “…etc.”.
libs/core/src/components/pds-row/pds-row.tsx (2)
94-104: Simplify guards; avoid unnecessary trim on literal tokens.Use presence + map membership; reduces checks and type noise.
- ...(this.colGap && { - '--row-gap-x': this.colGap !== undefined && this.colGap.trim() !== '' ? this.colGapMap[this.colGap] : '', - '--row-gap-y': this.colGap !== undefined && this.colGap.trim() !== '' ? this.colGapMap[this.colGap] : '', - }), + ...(this.colGap && this.colGapMap[this.colGap] && { + '--row-gap-x': this.colGapMap[this.colGap], + '--row-gap-y': this.colGapMap[this.colGap], + }), - ...(this.colGapBlock && { - '--row-gap-y': this.colGapBlock !== undefined && this.colGapBlock.trim() !== '' ? this.colGapMap[this.colGapBlock] : '', - }), + ...(this.colGapBlock && this.colGapMap[this.colGapBlock] && { + '--row-gap-y': this.colGapMap[this.colGapBlock], + }), - ...(this.colGapInline && { - '--row-gap-x': this.colGapInline !== undefined && this.colGapInline.trim() !== '' ? this.colGapMap[this.colGapInline] : '', - }), + ...(this.colGapInline && this.colGapMap[this.colGapInline] && { + '--row-gap-x': this.colGapMap[this.colGapInline], + }),
57-84: Consider mapping T‑shirt sizes via design tokens for consistency.Numeric tokens use CSS vars; T‑shirt sizes use rems. If Pine exposes size tokens (e.g., --pine-space-sm), prefer those for uniform theming.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/core/src/components.d.ts(9 hunks)libs/core/src/components/pds-box/docs/pds-box.mdx(3 hunks)libs/core/src/components/pds-box/pds-box.tsx(4 hunks)libs/core/src/components/pds-row/pds-row.tsx(4 hunks)libs/core/src/components/pds-row/readme.md(1 hunks)libs/core/src/utils/types.ts(2 hunks)
🧰 Additional context used
🪛 LanguageTool
libs/core/src/components/pds-row/readme.md
[style] ~19-~19: ‘in conjunction with’ might be wordy. Consider a shorter alternative.
Context: ... | The minimum height of the row. Used in conjunction with alignment props |...
(EN_WORDINESS_PREMIUM_IN_CONJUNCTION_WITH)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test-lint (20)
- GitHub Check: test-specs (22)
- GitHub Check: test-lint (22)
- GitHub Check: test-lint (18)
- GitHub Check: test-specs (18)
- GitHub Check: test-specs (20)
- GitHub Check: Cycode: Vulnerable Dependencies
🔇 Additional comments (5)
libs/core/src/utils/types.ts (1)
30-56: BoxSpacingType expansion looks good.Adding core tokens alongside T‑shirt sizes provides needed flexibility without breaking existing callers.
libs/core/src/components.d.ts (2)
8-16: Public API uses BoxSpacingType across components — good consistency.Imports/exports updated; spacing props now accept both T‑shirt and core tokens. Looks good.
637-637: Shadow prop typing vs docs — confirm aliases or update docs.Here
shadow?: BoxShadowSizeType(numeric tokens). Docs/examples often show'xs'|'sm'|'md'|'lg'. Please verify supported values and align types/docs, or add aliases.libs/core/src/components/pds-box/pds-box.tsx (2)
3-3: LGTM! Type import updated correctly.The import statement correctly reflects the type rename from
BoxTShirtSizeTypetoBoxSpacingType, aligning with the expanded spacing token support.
295-433: Margin property types updated correctly.All margin properties (marginBlockStart, marginInlineStart, marginInlineEnd, marginBlockEnd and their responsive variants) have been consistently updated to use
BoxSpacingType. The render method generates appropriate class names for these properties.
| | Size | Pixel Value | | ||
| |------|-------------| | ||
| | `"none"` | 0px | | ||
| | `"xxs"` | 2px | | ||
| | `"xs"` | 4px | | ||
| | `"sm"` | 8px | | ||
| | `"md"` | 16px | | ||
| | `"lg"` | 24px | | ||
| | `"xl"` | 32px | | ||
| | `"xxl"` | 40px | | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
T‑shirt spacing table appears inconsistent with runtime scale. Please verify and align.
Row’s colGapMap uses xxs=.25rem (4px), xs=.5rem (8px), sm=1rem (16px), md=1.5rem (24px), lg=2.25rem (36px), xl=3rem (48px), xxl=4rem (64px), which conflicts with this table (xxs=2px…xxl=40px). Confirm the source of truth (tokens vs. rem scale) and update Box/Row docs accordingly.
I'll verify the spacing table inconsistency by examining the codebase for the actual runtime token definitions.
🏁 Script executed:
#!/bin/bash
# Find Row colGapMap definition
rg -n "colGapMap" --type=ts --type=js -C5Length of output: 2465
🏁 Script executed:
#!/bin/bash
# Find spacing/gap token definitions in Box or core tokens
rg -n "xxs|xs.*px|gap.*map" --type=ts --type=js -g "*/tokens/*" -g "*/spacing*" -C2Length of output: 148
🏁 Script executed:
#!/bin/bash
# Find the pds-box.mdx file and check context around lines 50-60
fd "pds-box.mdx" --type=fLength of output: 49
🏁 Script executed:
#!/bin/bash
# Search for spacing scale or token definitions in config/design files
fd "spacing|tokens|scale" --type=f | head -20Length of output: 378
🏁 Script executed:
#!/bin/bash
# Look for any rem/px conversion or token definitions near pds-row or pds-box
rg -n "\.25rem|\.5rem|1rem|1\.5rem|2\.25rem|3rem|4rem" --type=ts --type=js -C3Length of output: 7029
🏁 Script executed:
#!/bin/bash
# Search for Box padding/margin token definitions
rg -n "pds-box.*padding|pds-box.*margin|Box.*spacing" --type=ts --type=jsx --type=tsx -C3Length of output: 27
🏁 Script executed:
#!/bin/bash
# Look for design token or spacing constant definitions
fd ".*token.*" --type=f | grep -E "\.(ts|js|json)$" | head -15Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find references to spacing scales (xxs, xs, sm, md, lg, xl, xxl)
rg -nP 'xxs|xs.*:\s*["\']?[\d.]+' --type=json --type=ts -C2 | head -40Length of output: 68
🏁 Script executed:
#!/bin/bash
# Check the actual pds-box.mdx content around the table
cat libs/core/src/components/pds-box/docs/pds-box.mdx | sed -n '40,70p'Length of output: 1074
🏁 Script executed:
#!/bin/bash
# Find pds-row component and colGapMap
fd "pds-row" -type dLength of output: 170
🏁 Script executed:
#!/bin/bash
# Look for spacing definitions in a CSS or token file
rg -n "xxs.*2px|xs.*4px" --type=css --type=json --type=ts -C2Length of output: 2996
🏁 Script executed:
#!/bin/bash
# Search for the actual spacing token source of truth
rg -n '"xxs"|"xs"|"sm"' --type=json -C5 | grep -A5 -B5 "px\|rem"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find the pds-row index file or main definition
rg -n "colGapMap\|gapMap" -C10 --type=ts --type=jsxLength of output: 27
🏁 Script executed:
#!/bin/bash
# Check for any spacing config or constants file
fd "config|constant|theme|design" --type=f -e ts -e js -e json | head -20Length of output: 313
🏁 Script executed:
#!/bin/bash
# Search more broadly for gap or spacing related maps
rg -n "gap\s*[:=].*{" --type=ts --type=jsx -A10 | head -50Length of output: 89
🏁 Script executed:
#!/bin/bash
# Look for the exact pixel values mentioned in the review
rg -n "2px|4px|8px|16px|24px|32px|40px" --type=ts --type=js --type=json | head -30Length of output: 3130
🏁 Script executed:
#!/bin/bash
# Find Box component implementation
fd "pds-box" -type dLength of output: 170
🏁 Script executed:
#!/bin/bash
# Check for spacing scale definition in a centralized location
rg -n "spacing.*scale|scale.*spacing" --type=ts --type=json -C5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for the exact colGapMap with rem values
rg -n "colGapMap|gapMap" -B5 -A15 --type=ts --type=jsLength of output: 3562
🏁 Script executed:
#!/bin/bash
# Check if pds-box has spacing prop or similar
rg -n "padding|margin|gap|spacing" libs/core/src/components/pds-box/ -C2Length of output: 206420
🏁 Script executed:
#!/bin/bash
# List contents of pds-box directory
find libs/core/src/components/pds-box -type f | head -20Length of output: 409
🏁 Script executed:
#!/bin/bash
# List contents of pds-row directory if it exists
find libs/core/src/components/pds-row -type f 2>/dev/null | head -20Length of output: 356
🏁 Script executed:
#!/bin/bash
# Check what's in the components directory
ls -la libs/core/src/components/ 2>/dev/null | head -20Length of output: 1135
🏁 Script executed:
#!/bin/bash
# Search for token or scale definitions
rg -n "export.*spacing|export.*scale|export.*token" --type=ts --type=js | grep -i "xxs\|scale\|spacing"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find any design tokens directory
find libs -name "*token*" -o -name "*scale*" 2>/dev/null | head -20Length of output: 482
🏁 Script executed:
#!/bin/bash
# Look for the actual spacing map with all sizes
rg -n "xxs.*:" --type=ts --type=json -B2 -A2 | head -60Length of output: 367
Update documentation: T-shirt size table has incorrect pixel values.
The pds-box.mdx spacing table shows xxs=2px, xs=4px...xl=32px, xxl=40px, but the actual SCSS tokens in pds-box.scss define xxs=4px, xs=8px...xl=40px (no xxl). Additionally, pds-row's colGapMap uses a different scale: xxs=4px...xl=48px, xxl=64px. Correct the documentation table to match the actual pds-box SCSS token values or clarify the separate scales used by Row vs. Box.
| @Prop() gap?: BoxSpacingType; | ||
|
|
||
| /** | ||
| * Defines the spacing between the box items at the XS breakpoint. | ||
| * @defaultValue none | ||
| */ | ||
| @Prop() gapXs?: BoxTShirtSizeType; | ||
| @Prop() gapXs?: BoxSpacingType; | ||
|
|
||
| /** | ||
| * Defines the spacing between the box items at the SM breakpoint. | ||
| * @defaultValue none | ||
| */ | ||
| @Prop() gapSm?: BoxTShirtSizeType; | ||
| @Prop() gapSm?: BoxSpacingType; | ||
|
|
||
| /** | ||
| * Defines the spacing between the box items at the MD breakpoint. | ||
| * @defaultValue none | ||
| */ | ||
| @Prop() gapMd?: BoxTShirtSizeType; | ||
| @Prop() gapMd?: BoxSpacingType; | ||
|
|
||
| /** | ||
| * Defines the spacing between the box items at the LG breakpoint. | ||
| * @defaultValue none | ||
| */ | ||
| @Prop() gapLg?: BoxTShirtSizeType; | ||
| @Prop() gapLg?: BoxSpacingType; | ||
|
|
||
| /** | ||
| * Defines the spacing between the box items at the XL breakpoint. | ||
| * @defaultValue none | ||
| */ | ||
| @Prop() gapXl?: BoxTShirtSizeType; | ||
| @Prop() gapXl?: BoxSpacingType; |
There was a problem hiding this comment.
🧩 Analysis chain
Type update looks good; verify CSS class support for new tokens.
The gap properties have been correctly updated to use BoxSpacingType. The render method generates class names by interpolating these values (e.g., pds-box-gap-${this.gap}). Ensure that corresponding CSS classes exist for all new numeric tokens ('025', '050', '100', etc.) that are now part of BoxSpacingType.
Run the following script to verify CSS class definitions:
🏁 Script executed:
#!/bin/bash
# Description: Check for CSS classes supporting numeric spacing tokens in pds-box styles
# Search for gap, margin, and padding classes with numeric tokens
rg -n --type=scss --type=css -C2 'pds-box-gap-(025|050|075|100|150|200|300|400|600|800)'
# Also check for SCSS variable or mixin definitions that might generate these classes
rg -n --type=scss -C3 '@each|@for.*gap.*(025|050|075|100|150|200|300|400|600|800)'Length of output: 57
---
🏁 Script executed:
```shell
#!/bin/bash
# Find CSS/SCSS style files related to pds-box
fd -e css -e scss | grep -i 'pds-box\|styles\|theme' | head -20
Length of output: 220
🏁 Script executed:
#!/bin/bash
# Locate the BoxSpacingType definition to understand available tokens
rg -n 'type BoxSpacingType|BoxSpacingType\s*=' --type=ts --type=tsx -A5Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for pds-box-gap class definitions in CSS/SCSS files (broader pattern)
fd -e css -e scss -x rg -l 'pds-box-gap' {}Length of output: 47
---
🏁 Script executed:
```shell
#!/bin/bash
# View the pds-box.scss file to see gap class definitions
cat libs/core/src/components/pds-box/pds-box.scss
Length of output: 15476
🏁 Script executed:
#!/bin/bash
# Search for BoxSpacingType definition in TypeScript files (broader search)
rg -n 'type BoxSpacingType' -A10Length of output: 474
🏁 Script executed:
#!/bin/bash
# Also check the mixins file
cat libs/core/src/components/pds-box/pds-box.mixins.scssLength of output: 1531
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full BoxSpacingType definition
sed -n '30,60p' libs/core/src/utils/types.ts
Length of output: 335
🏁 Script executed:
#!/bin/bash
# Double-check: search for any CSS classes with numeric tokens in gap helpers
rg -n 'pds-box-gap-(025|050|075|100|150|200|300|400|600|800)' libs/core/src/components/pds-box/Length of output: 0
Add CSS class definitions for all numeric spacing tokens in $gap-values map.
The TypeScript type BoxSpacingType now includes 19 numeric tokens ('025', '050', '100', '125', '150', '200', '250', '300', '350', '400', '450', '500', '550', '600', '650', '700', '750', '800'), but the corresponding CSS classes are missing from pds-box.scss. The render method generates class names like pds-box-gap-${this.gap}, which will fail to apply styles for any numeric token value.
Add entries for all numeric tokens to the $gap-values map (around line 732 in libs/core/src/components/pds-box/pds-box.scss), mapping each token to its corresponding CSS custom property (e.g., '025': var(--pine-dimension-025), etc.), ensuring the generated responsive gap classes include support for these values.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| '650': 'var(--pine-dimension-650)', | ||
| '700': 'var(--pine-dimension-700)', | ||
| '750': 'var(--pine-dimension-750)', | ||
| '800': 'var(--pine-dimension-800)' |
There was a problem hiding this comment.
Bug: Inconsistent Spacing Mapping Causes Maintenance Issues
The colGapMap in pds-row.tsx has inconsistent value mapping: t-shirt sizes use hardcoded rem values, while core spacing tokens use CSS variables. This means t-shirt sizes won't automatically reflect design system token changes, creating a maintenance discrepancy.
Description
Currently, the layout system is restricted to only T-shirt sizes. To ensure a more flexible experience, we're adding the core token values as well
padding,margin,gapFixes DSS-1561
Type of change
How Has This Been Tested?
Visit the Box or Row page, in the verify that the dropdown for any of the spacing props now contains the core dimension tokens
Test Configuration:
Checklist:
If not applicable, leave options unchecked.