Add default main colors to theme#12334
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!
|
frontend/lib/src/theme/getColors.ts
Outdated
There was a problem hiding this comment.
Will revisit when we handle background colors since grayColor is too light for light theme here 😞
There was a problem hiding this comment.
You mean the delta text? These should use the text colors (e.g. grayTextColor), not the main colors.
There was a problem hiding this comment.
No, I mean the grayColor - you don't think the line itself in the sparkline is too light? (Understand the confusion since one function currently controls both the delta text and the sparkline chart main color - so they use same color - will decouple/fix that when I get to text colors)
There was a problem hiding this comment.
I had the same feeling in the chart column color PR here that the grey chart feels a bit too light:
There was a problem hiding this comment.
And is the delta badge for light green the same as the combination of colors we use for the green badge (green background + green text)? This one also looks missing a bit of contrast.
There was a problem hiding this comment.
I guess we might want to refactor the metric component to use the green text color for the green text, the background color for the delta background and the main green color for the chart (instead of having a single metric positive / negative color)?
There was a problem hiding this comment.
Aah got it. So:
- For the gray main/line color, I agree it's a bit too light. I changed it to
gray60in the figma now, I think that's a good compromise, andgrayBackgroundallso uses that with 10% transparency (just like all the other background colors use the main color with 10% transparency). The nice thing about this is also that gray background then almost matches the background we have forst.codeetc. - Yes, agree we need to refactor this so we're using text colors for the delta text, background colors for the delta background + area chart area, and main colors for the line chart, bar chart, and the line of the area chart.
sfc-gh-mbarnes
force-pushed
the
default-main-colors
branch
from
13849ba to
0feec48
Compare
4 participants
Describe your changes
Add the following main color keys, with defaults for streamlit light/dark themes:
redColororangeColoryellowColorblueColorgreenColorvioletColorgrayColorMain colors apply to header dividers,
st.metricred/green/gray sparklines (line chart, line in area chart, bar chart) and chat message avatars (red/orange) for now - thus the snapshot updates in those categories. This sets the stage for configurable main colors (custom theming).Testing Plan