Manager: Added tokens and a dark color scheme for status colors #33081
Conversation
MichaelArestad
added
maintenance
|
View your CI Pipeline Execution ↗ for commit a5954ce
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new token-based color system for status indicators (positive, warning, negative, critical) to support both light and dark themes. The main changes include adding fgColor, bgColor, and borderColor token objects to the theme, refactoring status color references to use these tokens, and updating the sidebar tree component to dynamically apply theme-aware status colors.
Key Changes:
- Added
tokensexport with light/dark color schemes for foreground, background, and border colors - Refactored status color handling from hardcoded values to theme-based tokens
- Improved Tree.tsx rendering logic by extracting story/docs node handling into an early return
- Fixed invalid HSL syntax in dark theme (missing
%units)
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| code/core/src/theming/types.ts | Added token type imports and new fgColor, bgColor, borderColor properties to StorybookTheme interface |
| code/core/src/theming/themes/dark.ts | Fixed invalid HSL color syntax by adding missing percentage units |
| code/core/src/theming/tests/convert.test.js | Updated test expectations to match new HSL color format |
| code/core/src/theming/create.ts | Improved type safety (any → unknown) and simplified object spread syntax |
| code/core/src/theming/convert.ts | Added tokens to theme conversion and inverted ternary conditions for consistency |
| code/core/src/theming/base.ts | Added comprehensive tokens object with light/dark color schemes for status indicators |
| code/core/src/manager/utils/status.tsx | Converted statusMapping from constant to function to support dynamic theme colors |
| code/core/src/manager/globals/exports.ts | Added 'tokens' to theming package exports list |
| code/core/src/manager/components/sidebar/Tree.tsx | Refactored node rendering with early return for story/docs nodes and applied theme-based status colors |
| code/core/src/manager/components/sidebar/SearchResults.tsx | Updated to use getStatus function and fixed useEffect dependency array |
| code/core/src/components/components/Badge/Badge.tsx | Migrated from hardcoded color values to token-based theming system |
| export const getStatus = (theme: Theme, status: StatusValue) => { | ||
| const statusMapping: Record<StatusValue, [ReactElement | null, string | null]> = { | ||
| ['status-value:unknown']: [null, null], | ||
| ['status-value:pending']: [<LoadingIcons key="icon" />, 'currentColor'], | ||
| ['status-value:success']: [ | ||
| <svg key="icon" viewBox="0 0 14 14" width="14" height="14"> | ||
| <UseSymbol type="success" /> | ||
| </svg>, | ||
| 'currentColor', | ||
| ], | ||
| ['status-value:warning']: [ | ||
| <svg key="icon" viewBox="0 0 14 14" width="14" height="14"> | ||
| <UseSymbol type="warning" /> | ||
| </svg>, | ||
| theme.fgColor.warning, | ||
| ], | ||
| ['status-value:error']: [ | ||
| <svg key="icon" viewBox="0 0 14 14" width="14" height="14"> | ||
| <UseSymbol type="error" /> | ||
| </svg>, | ||
| theme.fgColor.negative, | ||
| ], | ||
| }; | ||
| return statusMapping[status]; |
There was a problem hiding this comment.
The statusMapping object is recreated on every call to getStatus, including all the React elements. This means JSX elements are being recreated unnecessarily on every render, even though only the 'warning' and 'error' colors depend on the theme.
Consider optimizing by defining the constant elements outside the function:
const constantStatusIcons = {
unknown: null,
pending: <LoadingIcons key="icon" />,
success: <svg key="icon" viewBox="0 0 14 14" width="14" height="14"><UseSymbol type="success" /></svg>,
};
export const getStatus = (theme: Theme, status: StatusValue) => {
const statusMapping: Record<StatusValue, [ReactElement | null, string | null]> = {
['status-value:unknown']: [constantStatusIcons.unknown, null],
['status-value:pending']: [constantStatusIcons.pending, 'currentColor'],
['status-value:success']: [constantStatusIcons.success, 'currentColor'],
['status-value:warning']: [/* create icon */, theme.fgColor.warning],
['status-value:error']: [/* create icon */, theme.fgColor.negative],
};
return statusMapping[status];
};| export const getStatus = (theme: Theme, status: StatusValue) => { | |
| const statusMapping: Record<StatusValue, [ReactElement | null, string | null]> = { | |
| ['status-value:unknown']: [null, null], | |
| ['status-value:pending']: [<LoadingIcons key="icon" />, 'currentColor'], | |
| ['status-value:success']: [ | |
| <svg key="icon" viewBox="0 0 14 14" width="14" height="14"> | |
| <UseSymbol type="success" /> | |
| </svg>, | |
| 'currentColor', | |
| ], | |
| ['status-value:warning']: [ | |
| <svg key="icon" viewBox="0 0 14 14" width="14" height="14"> | |
| <UseSymbol type="warning" /> | |
| </svg>, | |
| theme.fgColor.warning, | |
| ], | |
| ['status-value:error']: [ | |
| <svg key="icon" viewBox="0 0 14 14" width="14" height="14"> | |
| <UseSymbol type="error" /> | |
| </svg>, | |
| theme.fgColor.negative, | |
| ], | |
| }; | |
| return statusMapping[status]; | |
| // Static icons for statuses that do not depend on theme | |
| const constantStatusIcons: Record<StatusValue, ReactElement | null> = { | |
| 'status-value:unknown': null, | |
| 'status-value:pending': <LoadingIcons key="icon" />, | |
| 'status-value:success': ( | |
| <svg key="icon" viewBox="0 0 14 14" width="14" height="14"> | |
| <UseSymbol type="success" /> | |
| </svg> | |
| ), | |
| }; | |
| export const getStatus = (theme: Theme, status: StatusValue): [ReactElement | null, string | null] => { | |
| if (status === 'status-value:warning') { | |
| return [ | |
| <svg key="icon" viewBox="0 0 14 14" width="14" height="14"> | |
| <UseSymbol type="warning" /> | |
| </svg>, | |
| theme.fgColor.warning, | |
| ]; | |
| } | |
| if (status === 'status-value:error') { | |
| return [ | |
| <svg key="icon" viewBox="0 0 14 14" width="14" height="14"> | |
| <UseSymbol type="error" /> | |
| </svg>, | |
| theme.fgColor.negative, | |
| ]; | |
| } | |
| // For unknown, pending, success | |
| const icon = constantStatusIcons[status]; | |
| const color = | |
| status === 'status-value:pending' || status === 'status-value:success' | |
| ? 'currentColor' | |
| : null; | |
| return [icon, color]; |
There was a problem hiding this comment.
Open to suggestions here. Feel free to just make necessary changes.
There was a problem hiding this comment.
If statusMapping remains at the top level in the module, it'll be stable. Then getStatus can simply retrieve the key you want.
LMK if you want me to take over for code changes like that :)
There was a problem hiding this comment.
Take over please!
There was a problem hiding this comment.
I've memoized the output of the function so it should now be stable. Could you please just open the Storybook UI and check if everything works fine? I can't run the Vitest addon on my Linux box unfortunately :(
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughReplaces static status color mapping with a theme-aware getStatus(theme, status); introduces exported theme tokens (fgColor, bgColor, borderColor) and updates theme creation/conversion to merge tokens; updates Badge to use new tokens and inset border shadows; normalizes translucent color strings to CSS Color Level 4 syntax. Changes
Sequence Diagram(s)sequenceDiagram
participant Comp as Component (Tree / SearchResults / Badge)
participant Theme as useTheme()
participant GetStatus as getStatus(theme, status)
participant Render as UI Renderer
rect rgba(220,235,255,0.6)
Note over Comp,Theme: obtain current theme
Comp->>Theme: useTheme()
Theme-->>Comp: theme
Comp->>GetStatus: getStatus(theme, itemStatus)
GetStatus-->>Comp: [icon, color]
Comp->>Render: render icon, text color, Badge (boxShadow from borderColor)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
✨ Finishing touches
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-11-05T09:38:47.712ZApplied to files:
📚 Learning: 2025-11-05T09:37:25.920ZApplied to files:
⏰ 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). (2)
🔇 Additional comments (1)
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: 1
♻️ Duplicate comments (3)
code/core/src/theming/base.ts (1)
152-155: Dark theme status background colors are fully transparent.All four status background colors (
positive,warning,negative,critical) usehsl(101 100% 100% / 0)with alpha = 0, rendering them completely invisible. This issue was previously flagged and remains unresolved.code/core/src/manager/utils/status.tsx (1)
35-59: Performance optimization already suggested in prior review.The recreation of JSX elements on every call to
getStatushas been flagged in a previous review with a detailed optimization approach. Please address that feedback.code/core/src/manager/components/sidebar/Tree.tsx (1)
291-354: Inconsistent color handling already flagged.The null color handling inconsistency at line 318 has been noted in a previous review. Please address that feedback to align with the pattern used at line 516.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
code/core/src/components/components/Badge/Badge.tsx(1 hunks)code/core/src/manager/components/sidebar/SearchResults.tsx(3 hunks)code/core/src/manager/components/sidebar/Tree.tsx(5 hunks)code/core/src/manager/globals/exports.ts(1 hunks)code/core/src/manager/utils/status.tsx(2 hunks)code/core/src/theming/base.ts(2 hunks)code/core/src/theming/convert.ts(4 hunks)code/core/src/theming/create.ts(1 hunks)code/core/src/theming/tests/convert.test.js(2 hunks)code/core/src/theming/themes/dark.ts(2 hunks)code/core/src/theming/types.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.233Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.
Applied to files:
code/core/src/manager/globals/exports.ts
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: The useGlobals hook from storybook/manager-api returns a tuple where the third element (storyGlobals) is typed as Globals, not Globals | undefined. This means TypeScript guarantees it's always defined, making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/manager/globals/exports.ts
📚 Learning: 2025-09-21T15:08:50.897Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/theming/themes/dark.ts:34-36
Timestamp: 2025-09-21T15:08:50.897Z
Learning: In CSS Color Module Level 4, HSL syntax allows both percentage and numeric values for saturation and lightness components. The syntax `hsl(0 0 100 / 0.1)` is valid CSS4 - percentages are not required in modern HSL syntax. Both `hsl(0 0% 100% / 0.1)` and `hsl(0 0 100 / 0.1)` are correct.
Applied to files:
code/core/src/theming/base.tscode/core/src/theming/tests/convert.test.jscode/core/src/theming/themes/dark.ts
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/utils/status.tsxcode/core/src/manager/components/sidebar/SearchResults.tsx
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/core/src/theming/convert.tscode/core/src/manager/components/sidebar/SearchResults.tsx
🧬 Code graph analysis (5)
code/core/src/theming/create.ts (1)
code/core/src/theming/types.ts (2)
ThemeVarsPartial(6-6)ThemeVars(4-4)
code/core/src/manager/components/sidebar/Tree.tsx (4)
code/core/src/manager/components/sidebar/TreeNode.tsx (2)
DocumentNode(136-148)StoryNode(150-169)code/core/src/manager/utils/status.tsx (2)
getMostCriticalStatusValue(61-66)getStatus(35-59)code/core/src/manager/utils/tree.ts (1)
getLink(16-18)code/core/src/manager/components/sidebar/StatusButton.tsx (1)
StatusButton(91-93)
code/core/src/theming/convert.ts (1)
code/core/src/theming/base.ts (1)
tokens(104-167)
code/core/src/theming/types.ts (1)
code/core/src/theming/base.ts (1)
tokens(104-167)
code/core/src/manager/components/sidebar/SearchResults.tsx (1)
code/core/src/manager/utils/status.tsx (1)
getStatus(35-59)
⏰ 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). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (14)
code/core/src/theming/themes/dark.ts (1)
16-16: LGTM! CSS Color Level 4 syntax alignment.The color values have been updated to use the CSS Color Level 4 syntax with percentage notation and slash alpha separator. The semantic values remain unchanged (translucent white at 10% opacity), ensuring visual consistency.
Based on learnings
Also applies to: 36-36, 40-40
code/core/src/theming/tests/convert.test.js (1)
21-21: LGTM! Test expectations aligned with CSS Color Level 4 syntax.The test expectations have been correctly updated to match the new color notation format used throughout the theming system.
Also applies to: 71-71
code/core/src/manager/globals/exports.ts (1)
382-382: LGTM! Tokens export added to manager globals.The
tokensexport is correctly added to the storybook/theming globals, enabling manager-side access to the new theme token system introduced in base.ts.code/core/src/theming/base.ts (2)
28-28: LGTM! Border color updated to CSS Color Level 4 syntax.The border color notation has been updated to the modern CSS4 format while maintaining the same semantic value.
104-167: Tokens structure is well-organized with semantic naming.The new token system provides a clear, theme-aware approach to color management with
fgColor,bgColor, andborderColorgroups. The semantic keys (default,muted,accent,positive,warning,negative,critical) are intuitive and consistent across both themes.Note: The dark theme background color issue flagged separately needs resolution before this can be fully utilized.
code/core/src/theming/types.ts (1)
2-2: LGTM! Type definitions extended for token-based theming.The
StorybookThemeinterface now includes the newfgColor,bgColor, andborderColorproperties, properly typed using the token structure. This provides type safety and autocompletion for the new theming system.Also applies to: 74-76
code/core/src/theming/convert.ts (2)
4-4: LGTM! Token system integrated into theme conversion.The
tokensimport and conditional spread correctly merge theme-specific token values into the converted theme based on the base color scheme.Also applies to: 118-118
182-182: LGTM! Critical bug fix - theme conditionals now correct.The conditionals for syntax colors and addon actions theme are now properly aligned:
- Dark syntax colors are applied when
base === 'dark'- Dark chrome theme is applied when
base === 'dark'These appear to have been inverted previously, which would have caused incorrect theming in dark mode.
Also applies to: 189-189
code/core/src/manager/components/sidebar/SearchResults.tsx (2)
13-13: LGTM! Refactored to use theme-aware status rendering.The component now uses
getStatus(theme, status)for dynamic, theme-aware status icon and color resolution, replacing the previous static mapping. The theme is correctly obtained viauseTheme()hook.Also applies to: 16-16, 162-162, 181-181
176-176: LGTM! Correct dependency array for useEffect.The
apivariable is now correctly included in the useEffect dependency array, as it's used within the effect body (line 174). This ensures the effect properly re-runs when the api reference changes.code/core/src/components/components/Badge/Badge.tsx (1)
32-66: LGTM! Badge styling refactored to use token-based theming.All status cases now consistently use the new token system (
theme.fgColor.*,theme.bgColor.*,theme.borderColor.*), providing theme-aware color resolution. The uniformboxShadowpattern usingborderColortokens ensures consistent visual treatment across statuses.Note: Badge backgrounds in dark mode depend on the dark theme
bgColortoken values flagged in base.ts.code/core/src/theming/create.ts (1)
26-26: LGTM: Type safety improvement.Changing from
anytounknownis a good type safety practice.code/core/src/manager/components/sidebar/Tree.tsx (2)
222-222: LGTM: Theme integration.The addition of
useTheme()enables theme-aware status color resolution throughout the component.
423-440: LGTM: Consistent theme-aware color handling.The branch node rendering correctly uses
getStatus(theme, status)for theme-aware colors and properly checks truthiness before applying the style (line 440). This is the recommended pattern.
MichaelArestad
changed the title
| ); | ||
| const [icon, textColor] = getStatus(theme, statusValue); | ||
|
|
||
| return ( |
There was a problem hiding this comment.
Just a sanity check as I didn't expect you'd need to rework the tree structure. Is this new branch intentional or could it be caused by a merge autoresolution?
There was a problem hiding this comment.
It could have been via merge. @ghengeveld helped me with the initial work and I don't have a solid grasp of this specific set of changes.
| (item.type === 'story' && | ||
| !('children' in item && item.children) && | ||
| (!('subtype' in item) || item.subtype !== 'test')) || | ||
| item.type === 'docs' |
There was a problem hiding this comment.
To sum it up, we want a status only on docs and stories entries that do not contain child tests. Is that so?
There was a problem hiding this comment.
IIRC, status should be for:
- Groups (directory/component/story with tests)
- Stories and tests
The logic here is muddy to me.
There was a problem hiding this comment.
Feel free to make changes.
There was a problem hiding this comment.
I do think this is inconsistent with line 418 on the same file, but the Vitest addon on the monorepo does not run on my machine so I can't investigate this right now. :(
Co-authored-by: Steve Dodier-Lazaro <[email protected]>
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 35 | 20 | 🎉 -15 🎉 |
| Self size | 131 KB | 131 KB | 0 B |
| Dependency size | 3.15 MB | 2.81 MB | 🎉 -340 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
valentinpalkovic
added
the
needs qa
yannbf
removed
the
needs qa
Closes #
What I did
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This pull request has been released as version
0.0.0-pr-33081-sha-2e1e5f5d. Try it out in a new sandbox by runningnpx [email protected] sandboxor in an existing project withnpx [email protected] upgrade.More information
0.0.0-pr-33081-sha-2e1e5f5dm/fix-status-colors-maybe2e1e5f5d1763997612)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=33081Summary by CodeRabbit
New Features
Style
Usability
Tests
✏️ Tip: You can customize this high-level summary in your review settings.