fix: add semantic tokens #606
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
QuintonJason
force-pushed
the
fix/semantic-tokens
branch
from
12379b2 to
52e5b51
Compare
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
|
@coderabbitai review this pr |
|
@QuintonJason: I'll review this PR for you. Let me analyze the semantic token updates and Storybook dark mode additions. ✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request implements a comprehensive color token migration and dark mode theming system across the Pine design system. Changes include: adding a theme decorator to Storybook preview configuration with light/dark toggle functionality; updating preview head and body HTML files with dark theme CSS overrides; replacing grey-based color tokens with semantic tokens (e.g., text-secondary, background-subtle, border-subtle) across 25+ component SCSS files; modifying Storybook theme configuration files; and updating component documentation. No logic or control flow modifications were made; all changes are styling and theming related. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/core/src/components/pds-dropdown-menu/pds-dropdown-menu-item/pds-dropdown-menu-item.scss (1)
75-80: Missing semicolon on Line 79.The
width: 100%property is missing a trailing semicolon, which violates CSS/SCSS syntax and may cause linting failures.pds-link::part(link) { display: block; margin-inline: calc(var(--pine-dimension-xs) * -1); padding-inline: var(--pine-dimension-xs); - width: 100% + width: 100%; }libs/doc-components/src/components/docCanvas/docCanvas.css (1)
1-21: Use Pine semantic tokens for consistency with other doc-components styles.The
docCanvas.cssfile uses hardcoded hex values whiledocTokenTable.cssin the same package successfully uses Pine semantic tokens like--pine-dimension-xs,--pine-border, and--pine-color-grey-200. This creates inconsistency across the doc-components library.Refactor to use available Pine tokens:
:root { - --doc-canvas-background: #f9fafa; + --doc-canvas-background: var(--pine-color-background-subtle); - --doc-canvas-border: 1px solid #eceeef; + --doc-canvas-border: 1px solid var(--pine-color-border); - --doc-canvas-grey: #ececec; + --doc-canvas-grey: var(--pine-color-grey-200); - --doc-canvas-color-light: #ffffff; + --doc-canvas-color-light: var(--pine-color-background-default); - --doc-canvas-color: #333333; + --doc-canvas-color: var(--pine-color-text-default); } -[data-theme="dark"] { - --doc-canvas-background: #1a1a1a; - --doc-canvas-border: 1px solid #2d2d2d; - --doc-canvas-grey: #2d2d2d; - --doc-canvas-green: #628b83; - --doc-canvas-color-light: #000000; - --doc-canvas-color: #ffffff; -}
🧹 Nitpick comments (4)
libs/core/.storybook/preview-body.html (1)
7-12: Backgrounds now use semantic tokens; consider migrating remaining grey text tokenThe new backgrounds for
.sbdocs-wrapper,.sb-unstyled, andbodycorrectly usevar(--pine-color-background-*)tokens, which should behave well with your theme switching.There is still a direct use of
var(--pine-color-grey-700)for blockquote text (.sb-unstyled blockquote p). If the goal is to fully migrate away from--pine-color-grey-*, consider switching this to an appropriate semantic text token (e.g., a muted/secondary text token) to keep docs styling aligned with the rest of the system:- p { - color: var(--pine-color-grey-700); - } + p { + color: var(--pine-color-text-muted); + }Adjust the exact token as needed to match your visual spec.
Also applies to: 78-85, 89-91
libs/core/.storybook/preview-head.html (1)
8-175: Dark theme scaffolding for Storybook docs/canvas looks solidThe
[data-theme="dark"]block correctly uses CSS custom properties for core Pine surfaces and text, and the additional selectors for.sbdocs, argstables, and canvas give you a workable dark-docs experience without touching component logic.If you expect to iterate on dark-mode colors often, consider, over time, migrating more of the hard-coded hex values in these rules into shared design tokens or a small palette map so that Storybook docs and the main design tokens stay in sync. Not urgent, but it will simplify future tuning of dark mode.
libs/doc-components/src/components/docCanvas/docCanvas.css (2)
76-106: Dark theme hover relies on specificity override.The dark theme hover rule at lines 92-94 overrides the base hover rule at lines 102-106 through CSS specificity (
[data-theme="dark"]adds extra specificity). While functional, this creates a cascade that might be unclear to future maintainers.Consider grouping theme-specific styles together or documenting the override pattern:
.doc-canvas-action:hover { background: rgba(204, 204, 204, 0.28); box-shadow: inset 0 0 2px rgba(0, 0, 0, 0.3); color: #000; + + [data-theme="dark"] & { + color: #ffffff; + } } - -.doc-canvas-action:first-child { - border-right: 1px solid rgba(0, 0, 0, 0.1); - padding: 8px 15px; -} - -.doc-canvas-action:hover { - background: rgba(204, 204, 204, 0.28); - box-shadow: inset 0 0 2px rgba(0, 0, 0, 0.3); - color: #000; - - [data-theme="dark"] &:hover { - color: #ffffff; - } -}
135-144: Note: Pre-existing typo in class name.The class name
.doc-cavas-actions-togglehas a typo (should be.doc-canvas-actions-togglewith double 's'). While this is pre-existing and not introduced by this PR, it's worth noting for future cleanup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
libs/core/.storybook/PineTheme.js(1 hunks)libs/core/.storybook/preview-body.html(2 hunks)libs/core/.storybook/preview-head.html(1 hunks)libs/core/.storybook/preview.js(1 hunks)libs/core/src/components/pds-accordion/pds-accordion.scss(1 hunks)libs/core/src/components/pds-box/pds-box.scss(1 hunks)libs/core/src/components/pds-button/pds-button.scss(2 hunks)libs/core/src/components/pds-checkbox/pds-checkbox.scss(1 hunks)libs/core/src/components/pds-combobox/pds-combobox.scss(4 hunks)libs/core/src/components/pds-copytext/pds-copytext.scss(1 hunks)libs/core/src/components/pds-dropdown-menu/pds-dropdown-menu-item/pds-dropdown-menu-item.scss(1 hunks)libs/core/src/components/pds-filters/pds-filter/pds-filter.scss(3 hunks)libs/core/src/components/pds-input/docs/pds-input.mdx(1 hunks)libs/core/src/components/pds-input/pds-input.scss(2 hunks)libs/core/src/components/pds-link/pds-link.scss(1 hunks)libs/core/src/components/pds-loader/pds-loader.scss(1 hunks)libs/core/src/components/pds-modal/pds-modal-content/pds-modal-content.scss(1 hunks)libs/core/src/components/pds-progress/pds-progress.scss(1 hunks)libs/core/src/components/pds-switch/pds-switch.scss(1 hunks)libs/core/src/components/pds-table/pds-table-head-cell/pds-table-head-cell.scss(2 hunks)libs/core/src/components/pds-table/pds-table-head/pds-table-head.scss(1 hunks)libs/core/src/components/pds-tabs/pds-tab/pds-tab.scss(3 hunks)libs/core/src/components/pds-tabs/pds-tabs.scss(1 hunks)libs/core/src/components/pds-text/pds-text.scss(1 hunks)libs/core/src/components/pds-textarea/pds-textarea.scss(1 hunks)libs/doc-components/src/components/docArgsTable/docArgsTable.css(1 hunks)libs/doc-components/src/components/docCanvas/docCanvas.css(3 hunks)
⏰ 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). (6)
- GitHub Check: test-specs (22)
- GitHub Check: test-specs (20)
- GitHub Check: test-lint (18)
- GitHub Check: test-specs (18)
- GitHub Check: test-lint (20)
- GitHub Check: test-lint (22)
🔇 Additional comments (28)
libs/core/src/components/pds-loader/pds-loader.scss (1)
50-50: Semantic token migration looks correct.The change from
--pine-color-background-container-disabledto--pine-color-background-mutedis appropriate for this use case—the loader is an active component, not disabled. Usingmutedsemantics fits the neutral, recessed appearance needed for a loading animation container.Please verify the visual appearance of the typing loader matches expectations by toggling between light and dark modes in the Storybook theme switcher before merging.
libs/core/src/components/pds-dropdown-menu/pds-dropdown-menu-item/pds-dropdown-menu-item.scss (1)
33-36: Semantic token migration on hover state looks good.The update from
--pine-color-grey-150to--pine-color-background-mutedaligns with the PR's token migration strategy and should properly support both light and dark modes through CSS variable reassignment.Ensure that
--pine-color-background-mutedis properly defined in the ds-tokens package for both light and dark themes.libs/core/src/components/pds-filters/pds-filter/pds-filter.scss (1)
17-17: Semantic token migration looks consistent and logical.The changes follow a clear pattern: icon colors use
text-secondary, placeholder text usestext-tertiary, hover backgrounds usebackground-subtle, and hover borders useborder-hover. The token selections are contextually appropriate across both the main trigger button and the "more" trigger variant.Please verify that these semantic tokens are properly defined in the ds-tokens package and that they produce acceptable contrast ratios in both light and dark modes. You can confirm this by:
Checking the ds-tokens package for token definitions matching the names used here:
--pine-color-text-secondary--pine-color-text-tertiary--pine-color-background-subtle--pine-color-background-container-hover--pine-color-border--pine-color-border-hoverVisually testing the filter component in Storybook using the new dark mode theme toggle (mentioned in the PR summary) to confirm colors appear correct in both light and dark modes.
Running an accessibility checker or manually verifying that text-to-background contrast ratios meet WCAG AA or AAA standards for the updated color combinations.
Also applies to: 32-32, 39-40, 44-44, 86-88, 91-91, 96-97, 101-101
libs/core/src/components/pds-modal/pds-modal-content/pds-modal-content.scss (1)
11-12: Semantic token migration looks solid.The replacement of
--pine-color-grey-200with--pine-color-border-subtleis semantically clearer and aligns with the token migration objectives. All three border modifiers are updated consistently.Verify that
--pine-color-border-subtleis defined in the ds-tokens package and configured correctly for both light and dark themes.Also applies to: 16-16, 20-20
libs/core/src/components/pds-box/pds-box.scss (1)
123-127: Default border color migration to semantic token is appropriate.The change from
--pine-color-grey-300to--pine-color-borderon Line 124 improves clarity and consistency. The fallback pattern (var(--color-border-box, var(--pine-color-border))) is well-structured, allowing component-level overrides while using semantic defaults.Confirm that
--pine-color-borderis defined in ds-tokens with values suitable for both light and dark themes, and verify the token provides adequate visual contrast for bordered boxes.libs/core/src/components/pds-switch/pds-switch.scss (1)
113-128: Verify token migration rule application in disabled context.Line 122 applies
--pine-color-background-mutedto a hover state within a disabled input (input:disabled). However, the PR's documented token migrations explicitly state:--pine-color-background-container-disabled→--pine-color-background-muted(non-disabled contexts).This appears to be a semantic inconsistency—the token is being applied in a disabled context when the migration rules specify non-disabled contexts only.
Is this intentional? If so, the migration rules or documentation may need updating. If not, consider whether
--pine-color-background-mutedis the correct token for styling the disabled switch's toggle indicator on hover, or if a different semantic token should be used.libs/core/src/components/pds-link/pds-link.scss (1)
27-33: Confirm token definition in Tokens Studio before merging.The semantic token name
--pine-color-text-inverse-secondaryfollows Pine's naming convention (namespace-base-context-modifier) and is appropriately used for the secondary link hover state. However, this token is new and not defined anywhere in the repository—it must exist in the external Tokens Studio to function. Before merging this PR, verify that:
- The token
--pine-color-text-inverse-secondaryis properly defined in Tokens Studio and exported via Style Dictionary- The hover color meets WCAG AA contrast requirements in both light and dark themes
libs/core/src/components/pds-progress/pds-progress.scss (1)
23-23: Token migration is correct and well-established.The
--pine-color-background-mutedtoken is properly defined in the @kajabi-ui/styles design tokens package and supports dark mode theming via Tokens Studio exports. The semantic shift frombackground-container-disabledtobackground-mutedis appropriate for a progress bar track, which represents a neutral background state rather than a disabled UI element. This token is consistently used across multiple components in the codebase (tabs, switch, loader, dropdown menu, combobox) and integrates correctly with the Pine design system's light/dark theme support.libs/core/src/components/pds-checkbox/pds-checkbox.scss (1)
108-110: Token migration looks appropriate for disabled checkbox state.The change from
--pine-color-grey-300to--pine-color-borderis semantically clearer and aligns with the PR's semantic token migration. The disabled state styling remains consistent.Verify that
--pine-color-borderis properly defined with dark mode variants in ds-tokens to ensure consistent rendering in both themes.libs/core/src/components/pds-table/pds-table-head-cell/pds-table-head-cell.scss (1)
2-2: Verify semantic correctness of border-subtle token for cell borders.Line 2: Using
--pine-color-background-subtlefor a border is semantically unusual. Borders typically use border tokens. Verify this is intentional or if a border-specific token should be used instead.Line 47: The
text-active→text-strongchange is semantically appropriate for sortable/active states.Confirm whether
--pine-color-background-subtleis the correct choice for table cell borders, or if a--pine-color-border-subtleor similar token should be used instead.Also applies to: 47-47
libs/core/src/components/pds-accordion/pds-accordion.scss (1)
20-20: Inconsistent text color tokens between accordion states.Line 20: Open state uses
--pine-color-text-active(unchanged).
Line 32: Closed state changed from--pine-color-text-readonlyto--pine-color-text-muted.This creates an inconsistency—other PR changes migrate
text-activetotext-strong. Consider whether the open summary should also usetext-strongfor consistency, or iftext-activeis intentionally kept for the active/expanded state.Verify the intended semantics: should the open accordion summary use
text-stronginstead oftext-activefor consistency with other token migrations in this PR?Also applies to: 32-32
libs/core/src/components/pds-text/pds-text.scss (1)
5-6: Semantic token migration for underline decoration is appropriate.The change from
--pine-color-grey-600to--pine-color-text-tertiaryis semantically sound for a decorative element. Tertiary text color provides appropriate visual hierarchy for underline decorations.Ensure
--pine-color-text-tertiarymeets WCAG contrast requirements for text decoration in both light and dark themes.libs/core/src/components/pds-table/pds-table-head/pds-table-head.scss (1)
1-2: Border token consistency check (continued from pds-table-head-cell.scss).This file uses the same pattern—
--pine-color-background-subtlefor table borders. Confirm this is the intended design system convention for subtle borders, or if a dedicated border token should be used.This is part of the table border token migration. Please verify the
background-subtletoken choice alongside your response for pds-table-head-cell.scss.libs/core/src/components/pds-combobox/pds-combobox.scss (3)
31-67: Input styling token migration is appropriate.Line 36: Changing
text-activetotext-strongfor input text is semantically correct—user input should have strong contrast and visibility.
104-137: Selected/highlighted option styling updated to use semantic tokens.Lines 105 & 136: Changing from
--pine-color-grey-150to--pine-color-background-mutedfor selected options is semantically appropriate and supports dark mode theming.
145-157: Group label color updated to text-tertiary for appropriate visual hierarchy.Line 146: The change from
--pine-color-text-placeholderto--pine-color-text-tertiaryfor group labels is semantically sound. Group labels should be subtle/tertiary in the visual hierarchy.libs/core/src/components/pds-tabs/pds-tab/pds-tab.scss (3)
25-81: Tab base and availability variant color updates are semantically appropriate.Lines 29 & 89: The
--pine-color-grey-800→--pine-color-text-secondarymigrations for tab text provide proper semantic meaning for inactive tabs. The redundancy between base tab color (line 29) and availability variant (line 89) appears intentional for the variant-specific styling.
154-184: Filter tab variant updated to use background-muted.Line 156: Changing
--pine-color-background-container-disabledto--pine-color-background-mutedfor inactive filter tabs is semantically clearer and better aligns with the token system.
186-227: Pill tab variant color migration to text-muted.Line 193: Updating
--pine-color-text-readonlyto--pine-color-text-mutedfor inactive pill tabs is semantically appropriate. Note: The active pill state (line 210) still uses--pine-color-text-active—verify this is intentional or should be migrated totext-strongfor consistency with other PR changes.Confirm whether the active pill tab state (line 210) should also migrate from
text-activetotext-strongfor consistency with other token migrations in this PR.libs/core/src/components/pds-tabs/pds-tabs.scss (1)
28-36: Pill tablist border token migration looks goodUsing
var(--pine-color-border-subtle)for the pill tablist border aligns with the semantic token strategy and should behave well in both light/dark themes.libs/core/src/components/pds-input/pds-input.scss (1)
12-12: Input text + label usingtext-strongis consistent with token strategyAligning both
--pds-input-text-colorand.pds-input__labeltovar(--pine-color-text-strong)matches the semantic token migration and keeps primary input text appropriately emphasized.Also applies to: 185-187
libs/core/src/components/pds-input/docs/pds-input.mdx (1)
378-393: Prefix/suffix text docs correctly updated totext-mutedThe React, web component, and live examples all consistently use
var(--pine-color-text-muted)for prefix/suffix text, matching the semantic token migration and intended visual hierarchy.libs/core/src/components/pds-textarea/pds-textarea.scss (1)
130-146: Character counter color migration aligns with semantic tokensUsing
var(--pine-color-text-muted)for the character counter in the default state fits the muted/secondary text semantics, while state overrides (disabled/readonly) remain correctly in place.libs/core/.storybook/PineTheme.js (1)
9-13: Unified Storybook app backgrounds look reasonableSetting both
appBgandappContentBgto#f8f8f8should give a more consistent canvas/chrome appearance and plays nicely with the token-based body backgrounds configured elsewhere.libs/core/src/components/pds-copytext/pds-copytext.scss (1)
1-4: Copytext hover now driven by a semantic background tokenDefining
--copytext-color-background-hoverasvar(--pine-color-background-inset)and reusing it in the button hover state keeps the hover styling token-driven and consistent with other components.libs/core/src/components/pds-button/pds-button.scss (2)
166-188: LGTM!The filter button hover state correctly adopts the semantic token, maintaining consistency with the tertiary button changes.
153-164: Token migration is correct and consistent with established patterns.The
--pine-color-background-subtletoken is properly defined in the externalds-tokenspackage (listed as a dependency in catalog-info.yaml) and is actively used across multiple components including pds-input, pds-filters, and pds-table, confirming it's a valid semantic token that responds to theme changes. The migration from--pine-color-grey-100to this semantic token on lines 155 and 158 maintains visual consistency and aligns with the PR's semantic token adoption strategy.libs/core/.storybook/preview.js (1)
127-150: LGTM!The global theme configuration is well-structured with intuitive UI elements (sun/moon icons) and sensible defaults. The
dynamicTitlesetting enhances UX by displaying the active theme in the toolbar.
libs/doc-components/src/components/docArgsTable/docArgsTable.css
Outdated
Show resolved
Hide resolved
pixelflips
left a comment
pixelflips
left a comment
There was a problem hiding this comment.
See my comment for issues that I came across.
|
Cant see text in the invalid input: |
|
Text not flipping in layout docs exaample in dark mode. 
|
|
Popover doesn't have border to match Sage in dark mode. 
|
|
Disable switch circle disappears on hover: 
|
|
Text not readable inside textarea. May need to check other inputs as well. (showing text selected just to show there is content) 
|
pixelflips
left a comment
pixelflips
left a comment
There was a problem hiding this comment.
Phenomanal work my man! LGTM!
Update Semantic Tokens & Add Storybook Dark Mode Support
Summary
Updates Pine components to use new semantic tokens from ds-tokens and adds dark mode theme switching to Storybook for testing.
Fixes: DSS-34
Changes
Semantic Token Updates:
Storybook Dark Mode:
Token Migrations
--pine-color-text-active--pine-color-text-strong--pine-color-background-container-disabled--pine-color-background-muted(non-disabled contexts)--pine-color-grey-*Testing
Use the theme switcher in Storybook toolbar to toggle between light and dark modes.