Add default background colors to theme#12341
Conversation
mayagbarnes
added
security-assessment-completed
change:refactor
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
✅ PR preview is ready!
|
|
Hello, most color changes look good except for yellow where the background is much less visible now 🤔 |
|
Hi @bew! This was the color chosen by our design team, but this PR is part of a broader color updates project - I'll make note of this concern so we can review it and see if it should be adjusted after text color updates land 👍🏼 |
sfc-gh-mbarnes
force-pushed
the
default-background-colors
branch
from
f3a0d57 to
ace7833
Compare
sfc-gh-mbarnes
force-pushed
the
default-background-colors
branch
from
ace7833 to
a14f6c5
Compare
mayagbarnes
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR adds default background colors to Streamlit's theme system by introducing seven new background color keys (red, orange, yellow, blue, green, violet, gray) that are used across various UI elements including status elements, markdown backgrounds, badges, and metric components.
Key changes:
- Adds new background color properties to theme types and implementations
- Replaces hardcoded background color calculations with centralized theme-based colors
- Updates metric delta backgrounds and area chart colors to use the new system
Reviewed Changes
Copilot reviewed 13 out of 69 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
frontend/lib/src/theme/types.ts |
Adds status element background color properties to SpecialEmotionColors type |
frontend/lib/src/theme/primitives/colors.ts |
Adds new color variants yellow65 and blue65 to the color palette |
frontend/lib/src/theme/getColors.ts |
Centralizes background color logic and adds new utility functions |
frontend/lib/src/theme/getColors.test.ts |
Adds comprehensive tests for new background color functions |
frontend/lib/src/theme/emotionLightTheme/themeColors.ts |
Removes legacy background color definitions |
frontend/lib/src/theme/emotionDarkTheme/themeColors.ts |
Implements dark theme background colors with transparency |
frontend/lib/src/theme/emotionBaseTheme/themeColors.ts |
Implements light theme background colors with transparency |
frontend/lib/src/mocks/mockTheme.ts |
Updates mock theme to include new background colors |
frontend/lib/src/components/shared/StreamlitMarkdown/StreamlitMarkdown.test.tsx |
Updates tests to use centralized background color functions |
frontend/lib/src/components/elements/Metric/styled-components.ts |
Removes duplicate background color logic in favor of centralized function |
frontend/lib/src/components/elements/Metric/Metric.tsx |
Updates metric area charts to use new background colors |
frontend/lib/src/components/elements/Metric/Metric.test.tsx |
Updates test expectations for new background colors |
e2e_playwright/st_markdown_test.py |
Updates E2E test expectations for new background colors |
lukasmasuch
left a comment
lukasmasuch
left a comment
There was a problem hiding this comment.
LGTM 👍 A couple of comments, but nothing blocking the merge.
| // Status element colors | ||
| infoBg: string | ||
| dangerBg: string | ||
| successBg: string | ||
| warningBg: string |
There was a problem hiding this comment.
Do we even need these in our theme? I think it could simplify our theming if it only contains user-configurable colors + commonly used derived colors (borderColorLight, fadedTextXX). With the change in this PR, there isn't any difference anymore between infoBg and blueBackgroundColor. Same also applies to metricPositiveDeltaColor, metricPositiveDeltaColor and metricNeutralDeltaColor above. If we keep these colors in here it might make it a bit complicated to draw a line, e.g. when do we add a theme color vs just reuse an existing one? Why not add audioInputRecordColor? I think we might also want to enforce at some point that new configurable colors are the only allowed colors to be used within components.
There was a problem hiding this comment.
Funny you mention this - already working on a follow up PR that among other things, removes these intermediate color names - they are now just duplicates and the simplification will help for custom components v2.
Will keep this in mind as I continue 👍🏼
| purplebg: transparentize( | ||
| theme.colors[lightTheme ? "purple90" : "purple80"], | ||
| colors[lightTheme ? "purple90" : "purple80"], | ||
| lightTheme ? 0.9 : 0.7 |
There was a problem hiding this comment.
How does the rainbow look without the purple? Maybe we can just leave out this color to simplify (since it is only used by the rainbow, right?)
There was a problem hiding this comment.
It doesn't make a huge difference, but personally think it looks better including the purple
With Purple:
Without Purple:
There was a problem hiding this comment.
Something to consider as a follow-up: Related to the comment in https://github.com/streamlit/streamlit/pull/12341/files#r2311889546, it might make sense to only keep helper methods here that are actually needed to be shared across components. With the introduction of the new configurable colors, there might not be many benefits for keeping methods like getMarkdownBgColors within the theme module.
There was a problem hiding this comment.
personal taste: as a side-by-side comparison, the background colors in dark mode seems to be a bit less "colorful" / dull / less distinguishable (also see the MultiselectColumn tags above). 20% transparency might be a bit too much, and something between 10-20% would be a bit better. cc @jrieke
There was a problem hiding this comment.
Thats fair, will make a note to revisit (in any way, yellow bg was noted above as a concern)
Describe your changes
Add the following main color keys, with defaults for streamlit light/dark themes:
redBackgroundColororangeBackgroundColoryellowBackgroundColorblueBackgroundColorgreenBackgroundColorvioletBackgroundColorgrayBackgroundColorBackground colors apply to:
st.warning,st.success,st.info,st.error,st.exception) backgroundsst.badgebackgroundst.metricdelta background (red/green/gray)st.metricsparkline chart → area in area chart (red/green/gray)Updated (left) vs. Current (right):
Testing Plan