fix: update semantic token values in table#619
Conversation
✅ Deploy Preview for pine-design-system ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@coderabbitai review this pr |
|
@QuintonJason: I'll review the changes in this PR for you. ✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe DocTokenTable component is modified to support core token reference resolution. A new Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @libs/doc-components/src/components/docTokenTable/docTokenTable.tsx:
- Around line 112-140: The complexValueToString function destructures box-shadow
properties (x, y, blur, spread, color) without defaults, which can yield
"undefined" in the output; update complexValueToString to provide safe defaults
or filter out missing values when building the string: when handling Array items
and single object branches (the destructuring at the Array.map callback and the
object branch), assign sensible defaults (e.g., '' or '0' for numeric offsets
and '' for color) or construct the output by building an array
[x,y,blur,spread,color].filter(v => v != null && v !== '').join(' ') so missing
properties are omitted rather than rendered as "undefined".
🧹 Nitpick comments (1)
libs/doc-components/src/components/docTokenTable/docTokenTable.tsx (1)
88-110: Consider handling nested token references.The
lookupCoreValuefunction resolves a single reference but doesn't handle cases where a core token's value itself contains references to other core tokens (e.g.,{color.base.100}→{color.grey.100}→#f5f5f5). If your token system uses nested references, consider implementing recursive resolution.🔎 Example approach for recursive resolution
const lookupCoreValue = (refPath: string, cores: CoreTokens): any => { const parts = refPath.split('.'); let current: any = cores; for (const part of parts) { if (current && typeof current === 'object' && part in current) { current = current[part]; } else { return null; } } if (current && typeof current === 'object' && 'value' in current) { - return current.value; + const value = current.value; + // If the value itself contains references, resolve them recursively + if (typeof value === 'string' && value.includes('{')) { + return resolveReferences(value, cores); + } + return value; } return null; };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/doc-components/src/components/docTokenTable/docTokenTable.tsx
🔇 Additional comments (5)
libs/doc-components/src/components/docTokenTable/docTokenTable.tsx (5)
14-15: LGTM - Type definition is appropriate for dynamic JSON.Using
Record<string, any>is reasonable for the core tokens structure since it represents dynamically loaded JSON with arbitrary nesting.
86-86: LGTM - State addition is consistent.The
coreTokensstate follows the same pattern as the existingpineTokensstate.
142-154: LGTM - Reference resolution logic is sound.The regex pattern correctly identifies and replaces token references in the format
{path.to.value}. Returning the original match when a reference isn't found is a sensible fallback for documentation display.
160-165: LGTM - Core token loading is implemented correctly.The conditional loading of
core.jsonfor the semantic tier is appropriate and consistent with the existing token loading pattern on line 159. The existing try-catch block will handle any import failures.
246-249: LGTM - Reference resolution correctly achieves the PR objective.The conditional reference resolution is well-guarded and correctly placed in the rendering pipeline. This change ensures semantic token values display their resolved core values instead of references like
{color.grey.100}.
Description
Update semantic token values to show the core value instead of the reference.
Fixes #(issue)
Type of change
How Has This Been Tested?
Visit any of the semantic tokens views and verify the value
Test Configuration:
Checklist:
If not applicable, leave options unchecked.