Manager: Added tokens and a dark color scheme for status colors#32541
Manager: Added tokens and a dark color scheme for status colors#32541MichaelArestad wants to merge 8 commits into
Conversation
MichaelArestad
requested review from
jonniebigodes and
kylegach
as code owners
MichaelArestad
added
maintenance
|
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. 📝 WalkthroughWalkthroughCentralizes theming via new tokens (fgColor/bgColor/borderColor), injects tokens into Theme creation, replaces static statusMapping with theme-aware getStatus(theme, status), and updates Badge, Tree, and SearchResults to use useTheme() + getStatus for status rendering. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Component (Badge / Tree / SearchResults)
participant UH as useTheme()
participant GS as getStatus(theme, status)
participant R as Renderer
C->>UH: request theme
UH-->>C: theme (includes tokens)
C->>GS: getStatus(theme, item.status)
GS-->>C: [icon|null, color|null] (derived from theme.tokens)
C->>R: render status UI using fgColor / bgColor / borderColor
note right of GS `#E6F2FF`: getStatus replaces static mapping\nand centralizes token usage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
✨ Finishing touches
Comment |
|
View your CI Pipeline Execution ↗ for commit 6887a11
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new token-based design system for status colors in Storybook themes, providing semantic color tokens that work consistently across light and dark themes while maintaining backwards compatibility.
- Added a new
tokensobject with semantic color definitions for foreground, background, and border colors - Updated both light and dark themes to use the new token system instead of hardcoded colors
- Applied the new status color tokens to the Badge component and sidebar tree components
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| code/core/src/theming/types.ts | Adds token types to ThemeVars interface and imports tokens |
| code/core/src/theming/themes/light.ts | Updates light theme to use semantic tokens instead of direct color references |
| code/core/src/theming/themes/dark.ts | Updates dark theme to use semantic tokens instead of hardcoded colors |
| code/core/src/theming/create.ts | Minor code style improvements and type safety updates |
| code/core/src/theming/convert.ts | Integrates tokens into theme conversion and fixes theme selection logic |
| code/core/src/theming/base.ts | Defines the new semantic token system with light and dark variants |
| code/core/src/manager/utils/status.tsx | Refactors status mapping to use theme-aware colors |
| code/core/src/manager/globals/exports.ts | Exports tokens for external consumption |
| code/core/src/manager/components/sidebar/Tree.tsx | Updates to use new theme-aware status function |
| code/core/src/manager/components/sidebar/SearchResults.tsx | Updates search results to use new status system |
| code/core/src/components/components/Badge/Badge.tsx | Applies new semantic color tokens to Badge component |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Sidnioulz
force-pushed
the
a11y-consolidation
branch
from
d2cfdff to
05040c4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
code/core/src/theming/convert.ts (2)
181-181: Verify syntax color selection logic.A previous review flagged this line as potentially having inverted logic. The current implementation uses dark syntax colors when
base === 'dark', which appears semantically correct. However, if this reverses prior behavior, ensure it's intentional and doesn't break existing syntax highlighting.Run the following verification to check if syntax colors are being applied correctly:
#!/bin/bash # Check for any test files or stories that validate syntax highlighting theme selection rg -n "darkSyntaxColors|lightSyntaxColors|createSyntax" -A3 -B3 --type=ts --type=tsx -g '!node_modules'
188-188: Verify addon actions theme selection logic.A previous review noted this condition logic change. The current implementation spreads
chromeDarkwhenbase === 'dark', which appears semantically correct. However, if this reverses the prior logic, verify it's intentional and doesn't affect the addon actions panel appearance.code/core/src/theming/base.ts (2)
135-135: Transparent critical border color needs a visible value.The
criticalborderColor in the light theme has0alpha, making it completely transparent. This would result in no visible border for critical status items.Apply this diff to provide a visible border:
- critical: 'hsl(16 100% 100% / 0)', + critical: 'hsl(16 100% 64% / 0.15)',
154-157: Transparent background colors need visible values for dark theme status indicators.All four status background colors (
positive,warning,negative,critical) in the dark theme have0alpha, making them completely transparent. This would make status indicators invisible or very difficult to see against dark backgrounds.Apply this diff to provide visible backgrounds:
- positive: 'hsl(101 100% 100% / 0)', - warning: 'hsl(101 100% 100% / 0)', - negative: 'hsl(101 100% 100% / 0)', - critical: 'hsl(101 100% 100% / 0)', + positive: 'hsl(101 52% 24% / 0.15)', + warning: 'hsl(41 67% 24% / 0.15)', + negative: 'hsl(16 100% 24% / 0.15)', + critical: 'hsl(16 100% 24% / 0.2)',
🧹 Nitpick comments (3)
code/core/src/components/components/Badge/Badge.tsx (2)
53-59: Consider using token-based colors for consistency.The neutral case uses
fgColor.mutedbut still relies on conditionaltheme.basechecks and non-token properties for background and border. For consistency with the other status variants, consider using token-based colors:case 'neutral': { return { color: theme.fgColor.muted, - background: theme.base === 'light' ? theme.background.app : theme.barBg, - boxShadow: `inset 0 0 0 1px ${transparentize(0.8, theme.textMutedColor)}`, + background: theme.bgColor.muted, + boxShadow: `inset 0 0 0 1px ${theme.borderColor.muted}`, }; }
67-74: Consider migrating active case to token-based theming.The active case still uses legacy theme properties rather than the new token-based approach. For consistency with the rest of the Badge component's status variants, consider updating to use tokens:
Note: This may require adding an appropriate token for the "active" state in the tokens definition if one doesn't exist yet.
code/core/src/manager/utils/status.tsx (1)
35-59: Consider memoizing or caching the statusMapping to avoid recreation.The
statusMappingobject, including all React elements, is recreated on every call togetStatus. If this function is invoked frequently during renders (e.g., for each sidebar item), this could impact performance.Consider one of these approaches:
Option 1: Memoize per theme
const statusMappingCache = new WeakMap<Theme, Record<StatusValue, [ReactElement | null, string | null]>>(); export const getStatus = (theme: Theme, status: StatusValue) => { if (!statusMappingCache.has(theme)) { statusMappingCache.set(theme, { ['status-value:unknown']: [null, null], ['status-value:pending']: [<LoadingIcons key="icon" />, 'currentColor'], // ... rest of mapping }); } return statusMappingCache.get(theme)![status]; };Option 2: Move static elements outside
Only the color values depend on theme, so the icon elements could be created once:const icons = { 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 mapping: Record<StatusValue, [ReactElement | null, string | null]> = { ['status-value:unknown']: [null, null], ['status-value:pending']: [icons.pending, 'currentColor'], // ... }; return mapping[status]; };
📜 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(4 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/themes/dark.ts(1 hunks)code/core/src/theming/themes/light.ts(1 hunks)code/core/src/theming/types.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/core/src/manager/components/sidebar/SearchResults.tsxcode/core/src/manager/globals/exports.tscode/core/src/theming/base.tscode/core/src/theming/convert.tscode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/theming/themes/light.tscode/core/src/theming/types.tscode/core/src/theming/themes/dark.tscode/core/src/components/components/Badge/Badge.tsxcode/core/src/manager/utils/status.tsxcode/core/src/theming/create.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/core/src/manager/components/sidebar/SearchResults.tsxcode/core/src/manager/globals/exports.tscode/core/src/theming/base.tscode/core/src/theming/convert.tscode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/theming/themes/light.tscode/core/src/theming/types.tscode/core/src/theming/themes/dark.tscode/core/src/components/components/Badge/Badge.tsxcode/core/src/manager/utils/status.tsxcode/core/src/theming/create.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/core/src/manager/components/sidebar/SearchResults.tsxcode/core/src/manager/globals/exports.tscode/core/src/theming/base.tscode/core/src/theming/convert.tscode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/theming/themes/light.tscode/core/src/theming/types.tscode/core/src/theming/themes/dark.tscode/core/src/components/components/Badge/Badge.tsxcode/core/src/manager/utils/status.tsxcode/core/src/theming/create.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager/components/sidebar/SearchResults.tsxcode/core/src/manager/globals/exports.tscode/core/src/theming/base.tscode/core/src/theming/convert.tscode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/theming/themes/light.tscode/core/src/theming/types.tscode/core/src/theming/themes/dark.tscode/core/src/components/components/Badge/Badge.tsxcode/core/src/manager/utils/status.tsxcode/core/src/theming/create.ts
🧠 Learnings (3)
📚 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.ts
🧬 Code graph analysis (8)
code/core/src/manager/components/sidebar/SearchResults.tsx (1)
code/core/src/manager/utils/status.tsx (1)
getStatus(35-59)
code/core/src/theming/convert.ts (1)
code/core/src/theming/base.ts (1)
tokens(106-169)
code/core/src/manager/components/sidebar/Tree.tsx (2)
code/core/src/manager/utils/status.tsx (1)
getStatus(35-59)code/core/src/theming/base.ts (1)
color(3-44)
code/core/src/theming/themes/light.ts (2)
code/core/src/theming/base.ts (3)
tokens(106-169)color(3-44)typography(61-104)code/core/src/theming/types.ts (1)
ThemeVars(4-4)
code/core/src/theming/types.ts (1)
code/core/src/theming/base.ts (1)
tokens(106-169)
code/core/src/theming/themes/dark.ts (2)
code/core/src/theming/base.ts (3)
tokens(106-169)color(3-44)typography(61-104)code/core/src/theming/types.ts (1)
ThemeVars(4-4)
code/core/src/manager/utils/status.tsx (1)
code/core/src/shared/status-store/index.ts (1)
StatusValue(9-14)
code/core/src/theming/create.ts (1)
code/core/src/theming/types.ts (2)
ThemeVarsPartial(6-6)ThemeVars(4-4)
🔇 Additional comments (23)
code/core/src/components/components/Badge/Badge.tsx (2)
30-52: LGTM! Token-based theming applied consistently.The critical, negative, and warning status variants now correctly use token-based colors (fgColor, bgColor, borderColor) for consistent theming.
60-66: LGTM! Token-based theming applied.The positive status variant correctly uses token-based colors.
code/core/src/theming/types.ts (1)
2-2: LGTM! Token properties added to public theme interface.The StorybookTheme interface correctly extends with the new token-based color properties (fgColor, bgColor, borderColor), enabling type-safe access to the token system across the codebase.
Also applies to: 72-76
code/core/src/manager/globals/exports.ts (1)
381-381: LGTM! Tokens exported in Storybook theming globals.The addition of 'tokens' to the storybook/theming exports enables global access to the token system, which is essential for the token-based theming approach.
code/core/src/manager/components/sidebar/Tree.tsx (3)
37-37: LGTM! Import updated for theme-aware status resolution.The import correctly switches from the removed
statusMappingexport to the newgetStatusfunction, which enables theme-aware status color resolution.
229-229: LGTM! Theme-aware status resolution implemented.The Node component correctly retrieves the theme via
useTheme()and usesgetStatus(theme, statusValue)to compute status icons and colors dynamically based on the current theme.Also applies to: 303-303
402-402: LGTM! Status color extraction implemented correctly.The code correctly extracts the color value from
getStatus(theme, itemStatus)[1]with appropriate null handling for undefined status values.code/core/src/theming/convert.ts (2)
4-4: LGTM! Tokens imported for theme construction.
115-118: LGTM! Token-based colors spread into theme.The theme object correctly spreads token properties (fgColor, bgColor, borderColor) based on the theme base, making token-based colors available throughout the theme.
code/core/src/theming/create.ts (4)
14-14: LGTM! Improved type safety.Changing the Rest interface index signature from
anytounknownstrengthens type safety by requiring proper type assertions when accessing properties.
19-25: LGTM! Base theme validation improved.Computing the
basevariable upfront with fallback topreferredColorSchemeensures a valid theme key is always used, preventing potential undefined access issues.
26-31: LGTM! Theme inheritance logic simplified.With the validated
basevariable, the code can safely spreadthemes[vars.base]without a fallback, and directly assign thebaseproperty for cleaner, more readable code.
32-37: LGTM! Simplified property assignment.Using direct property assignment for
barSelectedColoris more conventional and readable than spreading a single-property object.code/core/src/manager/components/sidebar/SearchResults.tsx (4)
13-13: LGTM! Imports updated for theme-aware status resolution.The component correctly imports
useThemeand switches fromstatusMappingtogetStatus, aligning with the token-based theming approach.Also applies to: 16-16
173-173: LGTM! Theme context retrieved.
183-187: LGTM! Dependency array corrected.Adding
apito the useEffect dependency array is correct since it's referenced inside the effect. This prevents potential stale closure issues and satisfies React's exhaustive-deps rule.
192-192: LGTM! Theme-aware status icon retrieval.The code correctly uses
getStatus(theme, item.status)to retrieve the status icon with appropriate handling for undefined status values.code/core/src/theming/themes/dark.ts (3)
1-1: LGTM! Token imports and destructuring.The file correctly imports and destructures the dark theme tokens for use throughout the theme definition.
Also applies to: 4-4
10-18: LGTM! Color palette migrated to tokens.The color palette and UI colors correctly use token-based values (fgColor.accent, bgColor.muted/default, borderColor.default) instead of hard-coded hex values.
24-43: LGTM! All theme colors migrated to tokens.Text, toolbar, button, and input colors are all correctly using token-based values, completing the migration to the token-based theming system for the dark theme.
code/core/src/theming/themes/light.ts (1)
1-44: LGTM! Systematic migration to token-based theming.The migration from hardcoded color values to the token-based system is well-executed and consistent. All token references (
fgColor,bgColor,borderColor) align with the structure defined intokens.lightfrombase.ts.code/core/src/theming/base.ts (2)
30-30: LGTM! Valid CSS Color Level 4 syntax.The migration from
hsla(212, 50%, 30%, 0.15)tohsl(212 50% 30% / 0.15)is correct. The modern CSS Color Level 4 syntax is valid without percentage signs for saturation and lightness.Based on learnings
106-169: Verify token structure is complete and addresses all TODO comments.The new
tokensobject provides a solid foundation for theme-aware styling. However, there are several TODO comments indicating incomplete areas:
- Line 113: "TODO: add 'disabled'" for light theme fgColor
- Line 122: "TODO: add 'accent'? white or blue?" for light theme bgColor
- Line 144: "TODO: add 'disabled'" for dark theme fgColor
- Line 153: "TODO: add 'accent'? white or blue?" for dark theme bgColor
Please confirm whether these TODOs need to be addressed in this PR or if they're intentionally deferred to follow-up work. If deferred, consider opening issues to track them.
|
Not stale. Will likely make a new PR sans design tokens once the a11y branch is merged. |
MichaelArestad
changed the title
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
code/core/src/components/components/Badge/Badge.tsx:73
- The 'active' status case uses the old theming pattern with
theme.base === 'light'conditionals,darken(), andtransparentize()for colors, while other status cases use the new token-based colors. Consider adding 'active' status tokens to the tokens object and updating this case for consistency with the new theming approach.
case 'active': {
return {
color:
theme.base === 'light' ? darken(0.1, theme.color.secondary) : theme.color.secondary,
background: theme.background.hoverable,
boxShadow: `inset 0 0 0 1px ${transparentize(0.9, theme.color.secondary)}`,
};
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/manager/components/sidebar/Tree.tsx (1)
291-349: Status rendering refactor looks good; minor consistency improvement suggested.The migration to
getStatus(theme, statusValue)correctly implements theme-aware status colors for story/docs nodes. The logic properly handles null icons and maintains existing behavior.One minor inconsistency: Line 313 applies
textColorwithout checking for null, unlike lines 435 and 511 which guard against null values before setting the color style.Consider aligning with the pattern used elsewhere in this file:
<LeafNode // @ts-expect-error (non strict) - style={isSelected ? {} : { color: textColor }} + style={isSelected || !textColor ? {} : { color: textColor }} href={getLink(item, refId)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/manager/components/sidebar/Tree.tsx(5 hunks)code/core/src/theming/themes/dark.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/core/src/theming/themes/dark.ts
🧰 Additional context used
🧠 Learnings (1)
📚 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.tsx
🧬 Code graph analysis (1)
code/core/src/manager/components/sidebar/Tree.tsx (5)
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-api/root.tsx (1)
API(95-108)code/core/src/manager/components/sidebar/StatusButton.tsx (1)
StatusButton(91-93)
⏰ 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 (2)
code/core/src/manager/components/sidebar/Tree.tsx (2)
222-224: LGTM! Theme integration correctly implemented.The
useTheme()hook is properly placed at the component's top level and enables theme-aware status color rendering throughout the tree. This correctly supports the new dark color scheme functionality.
395-419: LGTM! Item and branch status colors correctly migrated to theme-aware API.The updates properly use
getStatus(theme, statusValue)for both item-level (line 396) and branch-level (line 418) status colors, with appropriate null checks before applying styles (lines 435, 511). This maintains existing behavior while enabling dark mode support.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
code/core/src/theming/base.ts (1)
104-167: Transparent/placeholder status tokens should be finalized before wider use.The overall
tokens.light/darkstructure looks good, but a few values appear unfinished and will likely cause invisible or misleading status styling:
- Line 133:
borderColor.criticalin the light theme useshsl(16 100% 100% / 0), which is fully transparent. That effectively removes the border for critical status instead of providing a distinct emphasis.- Lines 152–155:
bgColor.positive/warning/negative/criticalin the dark theme all usehsl(101 100% 100% / 0)— identical hue and fully transparent. That makes all status backgrounds invisible and indistinguishable, especially problematic now that these tokens are consumed by components likeBadgeand sidebar status colors.Given this PR’s a11y/contrast goals, these should be replaced with non‑zero alpha, hue‑appropriate values (e.g., subtle tinted backgrounds and borders around the existing status colors). The exact numbers are design‑driven, but keeping them non‑transparent and distinct per status is important.
🧹 Nitpick comments (2)
code/core/src/manager/components/sidebar/Tree.tsx (2)
291-354: Leaf story/docs status rendering looks good; consider a couple of minor refinements.The new branch for leaf stories and docs correctly:
- Computes a per-item
statusValueviagetMostCriticalStatusValue.- Derives
[icon, textColor]fromgetStatus(theme, statusValue).- Uses
StatusButtonwith a clearariaLabel.Two small cleanups to consider:
Only apply an inline
stylewhentextColoris meaningful. Right nowstyle={isSelected ? {} : { color: textColor }}will setcolor: null(for unknown statuses) orcolor: 'currentColor'(for pending/success), which is effectively a no-op:const leafStyle = !isSelected && textColor && textColor !== 'currentColor' ? { color: textColor } : undefined; … <LeafNode // @ts-expect-error (non strict) style={leafStyle} … />For a11y consistency with the non-leaf
itemStatusButton, consider addingrole="status"(or similar non-interactive role) to thisStatusButtonif it’s purely informational. Right now it’s rendered as a button without an action, which some screen readers may treat as an interactive control.
423-423: Simplifycolorcomputation for branch labels.
statusis always a non-emptyStatusValue(becausegetMostCriticalStatusValuereturns a concrete value), so the ternarystatus ? … : nullis redundant:const color = getStatus(theme, status)[1];If you explicitly want to skip coloring for unknown status, you can special-case it instead:
const color = status === 'status-value:unknown' ? null : getStatus(theme, status)[1];Either keeps the behavior clear without implying
statusmight be falsy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
code/core/src/components/components/Badge/Badge.tsx(1 hunks)code/core/src/manager/components/sidebar/Tree.tsx(5 hunks)code/core/src/theming/base.ts(2 hunks)code/core/src/theming/themes/dark.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- code/core/src/components/components/Badge/Badge.tsx
- code/core/src/theming/themes/dark.ts
🧰 Additional context used
🧠 Learnings (2)
📚 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.tsx
📚 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.ts
🧬 Code graph analysis (1)
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)
⏰ 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 (4)
code/core/src/theming/base.ts (1)
28-28: Border color update is fine and consistent.Using
hsl(212 50% 30% / 0.15)is valid CSS Color 4 syntax and fits the existing palette; no issues from a theming or compatibility standpoint.
Based on learningscode/core/src/manager/components/sidebar/Tree.tsx (3)
37-37: Status utilities import matches downstream usage.
getGroupStatus,getMostCriticalStatusValue, andgetStatusare all used later in this file; the import is correct and scoped appropriately.
222-222: UsinguseTheme()inNodeis appropriate for theme-aware status colors.Deriving status icon/text colors via
getStatus(theme, …)requires access to the current theme; callinguseTheme()here is the right place and keeps the component hook-safe.
401-401: Theme-based status icon/text color wiring is correct here.
itemStatusis derived viagetMostCriticalStatusValue, and passing it intogetStatus(theme, itemStatus)to obtain[itemIcon, itemColor]keeps branch/test leaf styling aligned with the new tokenized theme.
|
Closing in favor of #33081 |
What I did
tokensobject. This is testing the waters of how we might organize semantic tokens. It should not impact user theming.Badgecomponent to start.Todo: Once the general PR is approved, I will implement the new colors everywhere.
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 PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
Enhancements
Refactor