Skip to content

Provide ability to leverage CSS variables for status values provided by Vitest #31463

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: next
Choose a base branch
from

Conversation

JamesIves
Copy link
Contributor

@JamesIves JamesIves commented May 12, 2025

Closes #31462

What I did

Attaches CSS variables to status values, with fallbacks to their existing values. This allows things to work as-is with the ability to change them based on needs.

Screenshot 2025-05-12 at 12 54 06 PM Screenshot 2025-05-12 at 12 54 16 PM

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

  1. Set the CSS variable on the class provided to the button.
  2. When the tests error you'll now see the updated color.

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Make 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/core team 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>

Greptile Summary

Based on the provided information, I'll create a concise summary of the PR changes:

Introduces CSS variables for status value colors in the Storybook UI's sidebar, focusing on warning and error states to improve accessibility and theme customization.

  • Added --status-value-color-warning CSS variable with fallback to #A15C20 in code/core/src/manager/utils/status.tsx
  • Added --status-value-color-error CSS variable with fallback to brown in code/core/src/manager/utils/status.tsx
  • Maintains backward compatibility through fallback values for existing implementations
  • Addresses color contrast accessibility issues in the sidebar for Vitest addon status indicators

💡 (2/5) Greptile learns from your feedback when you react with 👍/👎!

@JamesIves JamesIves changed the title Status values Provide ability to leverage CSS variables for status values provided by Vitest May 12, 2025
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link

nx-cloud bot commented May 12, 2025

View your CI Pipeline Execution ↗ for commit 2cc4972.

Command Status Duration Result
nx run-many -t build --parallel=3 ✅ Succeeded 1m 43s View ↗

☁️ Nx Cloud last updated this comment at 2025-05-12 17:21:01 UTC

@ghengeveld
Copy link
Member

ghengeveld commented May 19, 2025

Hi @JamesIves,

Our theming system is currently not built around CSS variables but rather the Emotion ThemeProvider. We have plans to switch to a system based on CSS variables, hopefully this fall. If you have ideas or opinions around this I would love to get your feedback on that RFC.

Regarding this PR, I would rather not merge it as-is because it doesn't align with current practices and also makes it harder to holistically switch to CSS variables later. The proper solution (for now) would be to swap the CSS color strings for a color theme variable keys (probably warning and negative, see here), then have the components that use this value look it up from the theme. If needed, the components can use CSS color mixing to adjust for better contrast. It's more convoluted so I don't love it, but it's what fits the system we have.

@JamesIves
Copy link
Contributor Author

Is the warning theme colour something I can provide via the theme object? I'm really just looking for a way I can select the warning/error state so I can override it with my own variable.

@ghengeveld
Copy link
Member

@JamesIves Yes, you can override theme colors. Here's an example Storybook that does this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Vitest status values can cause color contrast issues on the sidebar
4 participants