feat: Add built-in color names support for chart color parameter#12698
feat: Add built-in color names support for chart color parameter#12698K-dash wants to merge 7 commits intostreamlit:developfrom
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
sfc-gh-lwilby
added
security-assessment-completed
impact:users
|
@sfc-gh-lwilby CI should be green now! Please review when you get a chance. Thanks 🙏 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds built-in color name support to Streamlit's chart functions (st.line_chart, st.bar_chart, st.area_chart, st.scatter_chart). Users can now use theme-aware color names like "red", "blue", "primary", etc., which automatically resolve to appropriate hex values based on the current theme.
- Added built-in color name recognition and theme-aware resolution system
- Integrated color name resolution into chart generation pipeline with proper priority handling
- Updated documentation for all chart functions to include built-in color usage examples
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
lib/streamlit/elements/lib/built_in_chart_utils.py |
Core implementation of built-in color name support with validation, resolution, and theme integration |
lib/streamlit/elements/vega_charts.py |
Integration of theme retrieval and color resolution into chart functions with updated documentation |
lib/tests/streamlit/elements/lib/built_in_chart_utils_test.py |
Comprehensive unit tests for color name validation, resolution, and theme handling |
lib/tests/streamlit/elements/vega_charts_test.py |
Integration tests verifying color resolution in Vega-Lite specs and regression tests for existing functionality |
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| ctx = get_script_run_ctx() | ||
| if ctx and hasattr(ctx, "session_state") and ctx.session_state: | ||
| return getattr(ctx.session_state, "theme", None) | ||
| return None |
There was a problem hiding this comment.
Bug: Theme retrieval function always returns None
The _get_theme() function attempts to retrieve a theme object from ctx.session_state.theme, but this attribute is never set anywhere in the Streamlit codebase. The session_state object is for user-defined variables, not system theme configuration. Theme colors are actually accessible via config.get_option("theme.redColor") etc. As a result, _get_theme() always returns None, and the feature falls back to hardcoded light-theme default colors instead of respecting the user's actual theme configuration. The feature advertised as "theme-aware" won't actually adapt to custom themes.
There was a problem hiding this comment.
@sfc-gh-lwilby
Thanks for catching this bug! You're absolutely right.
The _get_theme() function was attempting to access ctx.session_state.theme, which is never set anywhere in the codebase—session_state is for user-defined variables, not system configuration.
Fix:
- Removed _get_theme() and the ThemeLike protocol entirely
- Updated resolve_builtin_color_name() to use config.get_option("theme.{color}Color") directly
- Falls back to hardcoded defaults when config returns None (no custom theme)
Tests updated:
- Modified built_in_chart_utils_test.py to mock config.get_option instead of using MockTheme
- Added tests for both custom theme config and default fallback scenarios
- All existing tests pass
The feature now correctly respects user's custom theme configuration.
|
Hey @K-dash! Sorry for the delay. Could you add some screenshots of all different chart types with the different colors, so I can quickly check whether this looks correct? |
jrieke
added
the
missing:visual-demonstration
|
@jrieke Let me know if you need any additional screenshots or clarification. 
|
K-dash
force-pushed
the
feature/built-in-color-names-12694
branch
from
7169f37 to
cc15020
Compare
|
That's awesome, thank you so much for that screenshot, very helpful! Looks correct in my eyes, and I also roughly checked that we're using the same colors for the sparkline charts in One thing that is a little bit annoying/inconsistent is that for |
jrieke
added
status:product-approved
SummaryThis PR adds support for Streamlit's built-in color names in the Key Changes:
Code QualityStrengths:
Minor Issue: In Examples
--------
>>> resolve_builtin_color_name("red", theme)
"#ff4b4b" # theme.redColor
>>> resolve_builtin_color_name("primary", theme)
"#ff4b4b" # theme.primaryColor (prioritized)
>>> resolve_builtin_color_name("grey", theme)
"#808495" # theme.grayColor (grey is alias for gray)The value Question about The def _get_theme() -> Any:
"""Get the current theme from script run context.
Returns
-------
Any | None
Theme object if available, None otherwise
"""
ctx = get_script_run_ctx()
if ctx and hasattr(ctx, "session_state") and ctx.session_state:
return getattr(ctx.session_state, "theme", None)
return NoneI couldn't find evidence that Test CoverageExcellent test coverage with both unit tests and integration tests: Unit Tests (
|
sfc-gh-lwilby
added
ready-for-maintainer-review
…ion_state Remove _get_theme() which always returned None since ctx.session_state.theme is never set. Theme colors are now retrieved directly via config.get_option() in resolve_builtin_color_name().
|
|
||
|
|
||
| # Streamlit's built-in color names (7 basic palette colors + grey alias + primary) | ||
| BUILTIN_COLOR_NAMES: Final[frozenset[str]] = frozenset( |
There was a problem hiding this comment.
[suggestion: blocking] From the issue, it sounds like these colours are the same ones we are using for Markdown, it would be good to share the utils for the built-in colours amongst all places that use these built-in colours to ensure consistency.
| "sort_column": sort_column, | ||
| }, | ||
| # Chart styling properties | ||
| color=color_from_user, |
There was a problem hiding this comment.
This might need some updates as well, can you manually check whether the colour work after rows are added?
|
|
||
|
|
||
| def _resolve_color_names( | ||
| color: str | Color | list[Color] | None, |
There was a problem hiding this comment.
Have you considered handling the built-in colours in _get_color_encoding?
| # Array case: resolve each element | ||
| if isinstance(color, list): | ||
| return [ | ||
| resolve_builtin_color_name(cast("str", c)) |
There was a problem hiding this comment.
It seems like here c could be a tuple of RGB/RGBA here? I was just wondering if we could eliminate that case by doing this in _get_color_encoding so that we don't have to do a casting to string here. Perhaps not, though.
|
|
||
| # Single value case | ||
| if is_builtin_color_name(color): | ||
| return resolve_builtin_color_name(cast("str", color)) |
There was a problem hiding this comment.
Here it should be a str if is_builtin_color_name(color) is true. Alternatively, maybe just check if it is a tuple and if so, don't pass it to is_builtin_color_name?
For example,
if not isInstance(tuple, color) and is_builtin_color_name(color):
...
Then you should be able to update the input type to "str" for is_builtin_color_name" from Any` as well.
| ) | ||
|
|
||
|
|
||
| def is_builtin_color_name(color: Any) -> bool: |
There was a problem hiding this comment.
I think the type for color could be more specific, maybe either expand it to include Color (which includes Tuple), or handle the Tuple case outside of this function.
|
|
||
| # Default values (fallback when config returns None) | ||
| # Source: lib/streamlit/config.py theme color definitions (light theme defaults) | ||
| defaults: dict[str, str] = { |
There was a problem hiding this comment.
Do we already have this defined somewhere, where the logic could be shared with other areas that are using the built-in colors?
| return isinstance(color, str) and color.lower() in BUILTIN_COLOR_NAMES | ||
|
|
||
|
|
||
| def resolve_builtin_color_name(name: str) -> str: |
There was a problem hiding this comment.
Wondering actually if this whole function is not already a util somewhere since we use these built-in colours for other elements?
| "blue", | ||
| "green", | ||
| "violet", | ||
| "gray", |
| "", | ||
| None, | ||
| 123, | ||
| ["red"], |
There was a problem hiding this comment.
Maybe also a tuple like (255, 0, 0)?
| def test_resolve_with_custom_theme_config(self) -> None: | ||
| """Test color resolution when theme is configured via config.get_option().""" | ||
|
|
||
| def mock_get_option(key: str) -> str | None: |
There was a problem hiding this comment.
Can you use the existing util from tests.testutil import patch_config_options?
| assert resolve_builtin_color_name("primary") == "#ff4b4b" | ||
|
|
||
| def test_grey_alias(self) -> None: | ||
| """Test that 'grey' is correctly aliased to 'gray'.""" |
There was a problem hiding this comment.
Can you test upper case GREY as well?
| assert result == "mycolumn" | ||
|
|
||
|
|
||
| class TestBuiltinColorNamesConstant: |
There was a problem hiding this comment.
These tests seem unnecessary, they are testing the implementation.
| assert color_value == "#ff4b4b", ( | ||
| f"Expected default red color #ff4b4b, got {color_value}" | ||
| ) | ||
| assert color_value != "red", ( |
There was a problem hiding this comment.
This seems redundant with the assert above?
| [ | ||
| (st.line_chart, "line"), | ||
| (st.area_chart, "area"), | ||
| (st.bar_chart, "bar"), |
There was a problem hiding this comment.
Is there a reason to exclude area charts?
| ] | ||
| ) | ||
| def test_builtin_color_names_in_list_resolved( | ||
| self, chart_command: Callable, mark_type: str |
There was a problem hiding this comment.
It doesn't seem like the mark_type param is used.
| proto = self.get_delta_from_queue().new_element.arrow_vega_lite_chart | ||
| chart_spec = json.loads(proto.spec) | ||
|
|
||
| # Handle both layer (line_chart) and non-layer structures |
There was a problem hiding this comment.
We are only testing line chart here, so the if else isn't necessary.
| proto = self.get_delta_from_queue().new_element.arrow_vega_lite_chart | ||
| chart_spec = json.loads(proto.spec) | ||
|
|
||
| # Handle both layer (line_chart) and non-layer structures |
| df = pd.DataFrame([[20, 30]], columns=["a", "b"]) | ||
|
|
||
| # Hex color should pass through | ||
| st.line_chart(df, x="a", y="b", color="#AABBCC") |
There was a problem hiding this comment.
Should we also do the RGB/RGBA tuple?
| proto = self.get_delta_from_queue().new_element.arrow_vega_lite_chart | ||
| chart_spec = json.loads(proto.spec) | ||
|
|
||
| # Handle both layer (line_chart) and non-layer structures |
There was a problem hiding this comment.
The test only includes line charts.
| # If a column is named "red", it should be used as a column, not resolved as a color | ||
| df = pd.DataFrame([[20, 30, 1]], columns=["a", "b", "red"]) | ||
|
|
||
| st.line_chart(df, x="a", y="b", color="red") |
There was a problem hiding this comment.
Maybe vary these tests with different types. I agree we don't need to repeat these tests for every chart type though.
| with pytest.raises(StreamlitColumnNotFoundError): | ||
| st.bar_chart(df, x="A", y="B", sort="-nonexistent_column") | ||
|
|
||
| @parameterized.expand( |
There was a problem hiding this comment.
[suggestion] For all of these new unit tests, it might be better to parameterize them by color_val and then expected_proto_val, then it is easy to scan through the tests and see all of the cases of input mapped to output.
For checking the different chart types, you could run all the tests on all the chart types and then add subtests for each chart type.
|
@K-dash I've just been chatting with my theming-SME teammate, and it looks like we need to take a different approach for this to make sure that all of the theming works with this. I will share some more details afterwards. |
SummaryThis PR adds support for a small set of Streamlit “built-in” color names (e.g. Code Quality
Test Coverage
Backwards Compatibility
Security & Risk
Recommendations
VerdictCHANGES REQUESTED: The feature is solid and well-tested overall, but This is an automated AI review. Please verify the feedback and use your judgment. |
|
Hi @K-dash, thank you so much for your efforts in contributing this feature to streamlit! |
|
@mayagbarnes |
Describe your changes
Summary
Adds support for Streamlit's built-in color names in the
color=parameter of built-in chart functions (st.line_chart,st.bar_chart,st.area_chart,st.scatter_chart). Color names are resolved to theme tokens, enabling theme-aware color usage that automatically adapts to light/dark mode.In Scope
st.line_chart,st.bar_chart,st.area_chart,st.scatter_chartred,orange,yellow,blue,green,violet,gray/grey,primary"red"→theme.redColor)color=["blue", "green"],color=["primary", "#ff0000"]Out of Scope
rgb(255 0 0 / 0.5)(tracked in Support modern CSS color formats (consistently) #12681)primaryColorin config.tomlCore Implementation
Added built-in color name support in
built_in_chart_utils.py:BUILTIN_COLOR_NAMES: frozenset of 8 color names (red, orange, yellow, blue, green, violet, gray/grey, primary)is_builtin_color_name(): Validates if a string is a built-in color name (case-insensitive)resolve_builtin_color_name(): Resolves color names to theme tokens with fallback defaultsThemeLikeProtocol: Duck typing for theme objects_resolve_color_names(): Helper to resolve color names in arraysIntegrated theme-aware color resolution in chart generation:
generate_chart()to accept optionalthemeparameter_get_theme()helper invega_charts.pyto retrieve current themeDocumentation Updates
Color Resolution Priority
Theme Resolution
"red"→theme.redColor(e.g.,#ff4b4b)"red"→ default fallback (e.g.,#ff4b4b)"primary"→theme.primaryColorGitHub Issue Link (if applicable)
Closes #12694
Testing Plan
built_in_chart_utils_test.py:vega_charts_test.py:Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.