feat(island-ui): design tokens #16921
Conversation
WalkthroughThis pull request introduces significant changes to the color management system within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16921 +/- ##
==========================================
+ Coverage 35.60% 35.62% +0.01%
==========================================
Files 6934 6935 +1
Lines 146863 147277 +414
Branches 41755 41924 +169
==========================================
+ Hits 52297 52467 +170
- Misses 94566 94810 +244 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 20 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 33 Total Test Services: 0 Failed, 30 Passed Test ServicesThis report shows up to 10 services
|
…om/island-is/island.is into design_tokens_integration_research
…om/island-is/island.is into design_tokens_integration_research
…om/island-is/island.is into design_tokens_integration_research
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (16)
libs/island-ui/theme/tsconfig.json (1)
13-17: Consider adding JSDoc comments to document compiler options.Adding documentation about why these compiler options are needed would help future maintainers understand their purpose, particularly their relationship to design tokens and type safety requirements.
{ "extends": "../../../tsconfig.base.json", "include": [], "files": [], "references": [ { "path": "./tsconfig.lib.json" }, { "path": "./tsconfig.spec.json" } ], + /* Configuration for design tokens and enhanced type safety: + * - resolveJsonModule: Required for importing Figma design tokens from JSON + * - esModuleInterop: Ensures compatibility with different module formats + * - strict: Enables comprehensive type checking for better reliability + */ "compilerOptions": { "resolveJsonModule": true, "esModuleInterop": true, "strict": true } }libs/island-ui/theme/src/lib/tokens/tokens.ts (2)
1-2: Consider using ES modules syntax for JSON importsInstead of using
requirewith an eslint disable comment, consider using ES modules syntax for better TypeScript integration and maintainability.-// eslint-disable-next-line @typescript-eslint/no-var-requires -const figmaTokens = require('./figmaTokens.json') +import figmaTokens from './figmaTokens.json'Note: This requires proper TypeScript configuration with
resolveJsonModule: truein your tsconfig.json.
23-27: Add documentation and validation for typography tokensThe typography tokens lack documentation and validation. Consider:
- Adding JSDoc comments explaining the token structure and usage
- Validating that mobile/desktop breakpoints align with the theme's responsive system
- Adding type safety for typography values
/** Typography tokens from Figma * @example * { * "heading-xl": { * "fontSize": "48px", * "lineHeight": "1.2", * // ... other properties * } * } */libs/island-ui/theme/src/lib/colors/primitivesMapper.ts (2)
3-70: Well-structured color organizationThe color object follows a clean, logical hierarchy with consistent naming patterns. The structure supports effective tree-shaking and maintainability.
Consider documenting the color system structure in a README or design system documentation to help other developers understand the organization and usage patterns.
6-28: Simplify color mapping implementationThe current implementation is repetitive. Consider creating a helper function to reduce duplication and improve maintainability.
+const mapColorShades = (primitives: any, shades: string[]): ColorShade => { + return shades.reduce((acc, shade) => ({ + ...acc, + [shade]: primitives[shade].$value + }), {}) +} export const color = { primary: { - blue: { - 100: colorPrimitives.primary.blue['100'].$value, - 200: colorPrimitives.primary.blue['200'].$value, - // ... - }, + blue: mapColorShades(colorPrimitives.primary.blue, ['100', '200', '300', '400', '600']), // ... apply similar pattern to other colors }, // ... apply to other categories }Also applies to: 31-51, 54-68
libs/island-ui/theme/src/lib/colors/colors.ts (3)
59-60: Consider adding JSDoc comments for special colors.Document the purpose and usage of
transparentandcurrentColorconstants as they have special meaning in CSS.+/** CSS transparent keyword for fully transparent colors */ export const transparent = 'transparent' +/** CSS currentColor keyword that takes the color value from the current text color */ export const currentColor = 'currentColor'
96-128: Consider grouping tag colors into objects for better organization.The tag colors could be organized into objects to reduce repetition and improve maintainability.
-export const feedbackTagsBlueAccent = blue400 -export const feedbackTagsBlueBackground = blue100 -export const feedbackTagsBlueBorder = blue200 +export const feedbackTags = { + blue: { + accent: blue400, + background: blue100, + border: blue200, + }, + // ... other tag colors +} as const
135-194: LGTM! Consider adding dark mode support in the future.The color system is well-structured. For future enhancement, consider adding support for different color schemes (light/dark mode) by introducing a theme context.
This could be implemented by:
- Creating a theme context provider
- Wrapping these color values in getter functions that respect the current theme
- Adding color scheme variants for each token
libs/island-ui/theme/src/lib/tokens/figmaTokens.json (4)
1-1774: Consider enhancing token metadata for better maintainabilityThe token structure is well-organized, but could benefit from additional metadata to improve maintainability and documentation:
- Add
$descriptionfield to document the purpose and usage of each token- Add
$statusfield to manage token lifecycle (e.g., "stable", "deprecated")- Add
$deprecatedfield with replacement suggestions for deprecated tokens
7-240: Consider expanding the color palette for better accessibilityThe color system could benefit from additional shades to ensure sufficient contrast ratios for accessibility:
- Add 500 and 700 shades to provide more contrast options
- Consider adding a11y metadata to document contrast ratios
1208-1213: Standardize color value formatThe overlay color uses rgba format while other colors use hex. Consider standardizing to hex with alpha channel for consistency:
-"$value": "rgba(242, 247, 255, 70)" +"$value": "#f2f7ff4D"
1219-1772: Consider implementing a more systematic typography scaleThe typography system could be improved by:
- Using a modular scale for font sizes (e.g., 1.2 or 1.25 ratio)
- Defining line heights as ratios of font size for better maintainability
- Adding letter-spacing tokens for better control over text rendering
Example implementation:
{ "scale": { "ratio": 1.25, "base": 16 }, "lineHeight": { "tight": 1.2, "normal": 1.5, "loose": 1.8 } }libs/island-ui/theme/src/lib/theme.ts (4)
14-25: Ensure consistent spacing scale by addressing missing key '11'The
spacingobject skips the key11between10and12. For a consistent spacing scale, consider adding key11if it's required, or add a comment explaining the omission.
27-40: Review deprecated spacing entries for potential removalThere are several deprecated spacing entries (keys
14to31) in thespacingobject. If these values are no longer in use, consider removing them to improve code maintainability and reduce clutter.
107-108: Address TODO comments related to border radius valuesThe following border radius definitions have TODO comments indicating the need for designer input or updates to Figma:
- Lines 107-108: Confirm with designers whether to keep
standardandlargeradius values.- Lines 111-112: Determine if
xlandcircleradius values should be added to Figma.Please consult with the design team to finalize these values.
Would you like assistance in coordinating with designers or opening a GitHub issue to track these tasks?
Also applies to: 111-112
122-122: Clarify the focus border color with design teamThe
focusborder color is set tocolor.red200, with a comment questioning its alignment with design guidelines:focus: color.red200, // Ask designer what the border colors mean in sync with this?Please consult with the designers to confirm the appropriate border color for focus states to ensure consistency and adherence to the design system.
Would you like assistance in proposing an alternative or opening a GitHub issue to address this?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
libs/island-ui/theme/src/index.ts(1 hunks)libs/island-ui/theme/src/lib/colors.ts(0 hunks)libs/island-ui/theme/src/lib/colors/colors.ts(1 hunks)libs/island-ui/theme/src/lib/colors/primitivesMapper.ts(1 hunks)libs/island-ui/theme/src/lib/theme.ts(4 hunks)libs/island-ui/theme/src/lib/tokens/figmaTokens.json(1 hunks)libs/island-ui/theme/src/lib/tokens/tokens.ts(1 hunks)libs/island-ui/theme/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- libs/island-ui/theme/src/lib/colors.ts
🧰 Additional context used
📓 Path-based instructions (7)
libs/island-ui/theme/src/index.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/island-ui/theme/src/lib/colors/colors.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/island-ui/theme/src/lib/colors/primitivesMapper.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/island-ui/theme/src/lib/theme.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/island-ui/theme/src/lib/tokens/figmaTokens.json (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/island-ui/theme/src/lib/tokens/tokens.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/island-ui/theme/tsconfig.json (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (5)
libs/island-ui/theme/src/index.ts (1)
3-3: LGTM! Verify tree-shaking impact.
The reorganization of color exports improves code organization while maintaining the public API. However, let's verify that the new structure doesn't impact tree-shaking capabilities.
Let's analyze the import/export chain to ensure proper tree-shaking:
✅ Verification successful
Let me verify the tree-shaking impact by checking the actual exports in the colors file.
Tree-shaking is properly maintained in the new structure
The code inspection shows that tree-shaking capabilities are well preserved in the new structure:
- Individual color tokens are exported as named constants
- No intermediary re-exports using
export *in the colors.ts file - The barrel file's
export *is acceptable as it's the public API entry point - Many files in the codebase import specific colors, indicating effective tree-shaking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the export chain and usage patterns
# Expected: Should see clean export chains without * exports in colors.ts
# Check the export patterns in the new colors file
echo "Checking export patterns in colors.ts:"
ast-grep --pattern 'export const $$ = $$' libs/island-ui/theme/src/lib/colors/colors.ts
# Check if any files are importing specific colors (good for tree-shaking)
echo "Checking specific color imports:"
rg -l "import \{.*\} from.*island-ui/theme"
Length of output: 53904
Script:
#!/bin/bash
# Check the actual exports in the colors file
echo "Checking colors.ts exports:"
cat libs/island-ui/theme/src/lib/colors/colors.ts
# Check if there are any * exports in the chain
echo -e "\nChecking for * exports in the chain:"
rg "export \*" libs/island-ui/theme/src/
Length of output: 8955
libs/island-ui/theme/tsconfig.json (1)
13-17: LGTM! Verify inheritance from base config.
The compiler options align well with the module's needs, particularly for importing design tokens from JSON files and ensuring type safety.
Let's verify there are no conflicting compiler options in the base config:
✅ Verification successful
Compiler options are compatible with base configuration
The compiler options in libs/island-ui/theme/tsconfig.json are compatible with the shared configuration:
resolveJsonModuleis not set in the base config, allowing it to be enabled locallyesModuleInteropmatches the base config's setting (true)strictmatches the base config's setting (true)
The changes are safe to use as they maintain consistency with the base configuration while enabling JSON module support needed for design tokens.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the base tsconfig for conflicting compiler options
cat ../../../tsconfig.base.json | jq '.compilerOptions | select(.resolveJsonModule != null or .esModuleInterop != null or .strict != null)'
Length of output: 201
Script:
#!/bin/bash
# Let's first find the base tsconfig file
fd tsconfig.base.json
# Then check for any tsconfig files that might be referenced
fd tsconfig.json --exec cat {} \; | grep -B2 -A2 "extends"
Length of output: 41764
Script:
#!/bin/bash
# Let's check the content of the base tsconfig file
cat tsconfig.base.json
Length of output: 52922
Script:
#!/bin/bash
# Let's check the content of tsconfig.shared.json since base config extends from it
cat tsconfig.shared.json
Length of output: 767
libs/island-ui/theme/src/lib/colors/primitivesMapper.ts (1)
6-68: Verify color shade sequence
There are gaps in the numerical sequence of color shades (e.g., missing 500, 700). Please verify if this is intentional.
✅ Verification successful
Based on the shell script results, I can see that the color shade sequence is consistently implemented across the design system. The gaps in the numerical sequence (missing 500, 700) appear to be intentional as:
- The same pattern of shades (100, 200, 300, 400, 600) is consistently used across all primary and secondary colors (blue, red, blueberry, purple, rose)
- The design tokens file shows these specific shades are mapped to semantic tokens used throughout the application
- The only variations are:
- dark color having an additional 350 shade
- mint color having an additional 800 shade
- white color only having 400 shade
The color shade sequence is intentionally designed and consistently implemented
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in color shade numbering across the design system
# Search for other files that might define or use these color shades
rg -g '*.{ts,tsx,js,jsx}' -A 1 -B 1 '(100|200|300|400|500|600|700|800)' ./libs/island-ui/theme/
# Check design token definitions
fd -e json -e ts "tokens" ./libs/island-ui/theme/ --exec cat {} \; | grep -A 5 -B 5 "primary\|secondary\|accent"
Length of output: 63003
libs/island-ui/theme/src/lib/colors/colors.ts (2)
26-44: LGTM! Secondary colors are well-organized.
The secondary colors follow a consistent naming pattern and implementation approach.
62-77: LGTM! Well-structured semantic color definitions.
Background and border colors effectively reuse primary and secondary colors, promoting consistency across the theme.
…om/island-is/island.is into design_tokens_integration_research
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (2)
libs/island-ui/theme/src/lib/tokens/tokens.ts (1)
4-6: Add const assertions and extract magic valuesImprove maintainability and type safety of the primitives extraction.
+const PRIMITIVES_SECTION = '00 Primitives' as const; +const DEFAULT_MODE = 'Mode 1' as const; +const PRIMITIVES_INDEX = 0 as const; -const primitives = figmaTokens[0]['00 Primitives'].modes['Mode 1'] +const primitives = figmaTokens[PRIMITIVES_INDEX][PRIMITIVES_SECTION].modes[DEFAULT_MODE]libs/island-ui/theme/src/lib/theme.ts (1)
4-5: Consider using more specific imports for better tree-shaking.Instead of importing everything from colors using
*, consider importing only the needed color tokens for better tree-shaking and bundle optimization.-import * as color from './colors/colors' +import { blue200, red200, /* other specific colors */ } from './colors/colors'
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
libs/island-ui/theme/src/lib/colors/primitivesMapper.ts(1 hunks)libs/island-ui/theme/src/lib/theme.ts(4 hunks)libs/island-ui/theme/src/lib/tokens/tokens.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/island-ui/theme/src/lib/colors/primitivesMapper.ts
🧰 Additional context used
📓 Path-based instructions (2)
libs/island-ui/theme/src/lib/theme.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/island-ui/theme/src/lib/tokens/tokens.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (1)
libs/island-ui/theme/src/lib/tokens/tokens.ts (1)
1-23: Verify tree-shaking effectiveness
The current implementation might impact tree-shaking due to the use of require. Consider:
- Using named exports for individual tokens
- Enabling proper tree-shaking through modern import syntax
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
libs/island-ui/theme/src/lib/theme.ts (2)
10-36: Add type safety for spacing primitive valuesThe fallback pattern
spacingPrimitives['x'].$value ?? UNIT * ncould benefit from type safety to prevent typos in spacing keys.Consider adding a type-safe accessor:
+type SpacingKey = keyof typeof spacingPrimitives; +const getSpacingValue = (key: SpacingKey, fallback: number) => + spacingPrimitives[key].$value ?? fallback; export const spacing = { - 0: spacingPrimitives['0'].$value ?? UNIT * 0, + 0: getSpacingValue('0', UNIT * 0), // Apply similar pattern to other values }
103-109: Ensure consistent usage of radius tokensWhile waiting for designer feedback, consider improving type safety and consistency:
- Use TypeScript const assertions for hardcoded values
- Add JSDoc comments explaining the temporary nature of hardcoded values
radius: { standard: `${radius.xs.$value ?? 4}px`, large: `${radius.default.$value ?? 8}px`, md: `${radius.md.$value ?? 12}px`, lg: `${radius.lg.$value ?? 16}px`, - xl: '24px', + /** @todo Replace with design token once added to Figma */ + xl: '24px' as const, - circle: '50%', + /** @todo Replace with design token once added to Figma */ + circle: '50%' as const, full: `${radius.full.$value ?? 9999}px`, },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
libs/island-ui/theme/src/lib/theme.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/island-ui/theme/src/lib/theme.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (2)
libs/island-ui/theme/src/lib/theme.ts (2)
4-5: LGTM: Import changes align with design token system
The updated imports properly separate concerns and support tree-shaking by importing specific tokens.
81-81: LGTM: Typography uses design tokens with proper fallbacks
The font family configuration correctly uses the primary font token and provides appropriate fallback fonts.
…om/island-is/island.is into design_tokens_integration_research
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
libs/island-ui/theme/src/lib/theme.ts (2)
11-32: Consider strengthening the fallback patternWhile using design tokens with fallbacks is good, the current implementation might hide token mismatches. Consider adding development-time validation:
+const validateSpacingToken = (token: string, fallback: number) => { + if (process.env.NODE_ENV === 'development' && + !spacingPrimitives[token]?.$value) { + console.warn(`Missing spacing token: ${token}`); + } + return spacingPrimitives[token]?.$value ?? fallback; +} export const spacing = { - 0: spacingPrimitives['0'].$value ?? UNIT * 0, + 0: validateSpacingToken('0', UNIT * 0), // Apply to other values similarly
99-105: Enhance TODO handling for border radius valuesWhile the TODOs correctly identify missing Figma tokens, consider:
- Creating a tracking issue for the missing tokens
- Adding a temporary constant for these values:
+const PENDING_RADIUS_TOKENS = { + xl: 24, + circle: '50%' +} as const; radius: { standard: `${radius.xs.$value ?? 4}px`, large: `${radius.default.$value ?? 8}px`, md: `${radius.md.$value ?? 12}px`, lg: `${radius.lg.$value ?? 16}px`, - xl: '24px', // TODO: should be added to figma? - circle: '50%', // TODO: should be added to figma? + xl: `${PENDING_RADIUS_TOKENS.xl}px`, // Pending Figma token + circle: PENDING_RADIUS_TOKENS.circle, // Pending Figma token full: `${radius.full.$value ?? 9999}px`, },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
libs/island-ui/theme/src/lib/theme.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/island-ui/theme/src/lib/theme.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (2)
libs/island-ui/theme/src/lib/theme.ts (2)
4-5: LGTM: Import changes enhance modularity
The updated imports properly separate concerns and support effective tree-shaking.
77-77: LGTM: Typography configuration uses design tokens
The font family configuration properly uses design tokens while maintaining robust fallbacks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
libs/island-ui/theme/src/lib/theme.ts (2)
9-9: Clarify the scope of upcoming spacing updates.The TODO comment lacks specificity about what changes are planned. This makes it difficult for other developers to understand what to expect or avoid making conflicting changes.
Consider adding more details about:
- What aspects of spacing will be updated
- Whether current changes might be affected
- Expected timeline for the updates
100-106: Make TODOs more actionable with specific tasks.The current TODOs lack clear next steps. Consider updating them to be more specific:
-// TODO: check with designers to keep as standard +// TODO(design-system): Confirm with designers if 4px should remain as the standard radius token -// TODO: should be added to figma? +// TODO(design-system): Add xl (24px) and circle (50%) radius tokens to Figma design tokens
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
libs/island-ui/theme/src/lib/theme.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/island-ui/theme/src/lib/theme.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (2)
libs/island-ui/theme/src/lib/theme.ts (2)
4-5: LGTM! Import changes improve tree-shaking.
The more specific import paths and separation of tokens enhance modularity and tree-shaking capabilities.
78-78: LGTM! Font family configuration follows design token system.
The implementation properly uses the new token system while maintaining fallback fonts for cross-browser compatibility.
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor