-
Notifications
You must be signed in to change notification settings - Fork 121
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
feat(goal): dark mode with theme controls #1299
Conversation
It looks great, Nick!
All else being equal, more cohesion like shared logic and styling would be better than less. This way, the shared part could be lifted upwards from xy chart and placed in some |
Nick, big improvement over the original all around 🎉 Looks like the API just needs a couple of pieces of documentation due to policy |
Maybe it's just me but the tick labels are too faint on the changed mocks, though no worse than Cartesian axis labels, and more readable than Cartesians in that the font size is larger (as befit this chart type). @markov00 would it be useful to negotiate with Design a more contrasty color for the axis tick labels for both Cartesians and the bullet? |
... @markov00 @nickofthyme see also the bullet graph specification, I think we should stick to something that's perceptually very close to it, not just for readability, a11y and balance but also for spec adherence https://www.perceptualedge.com/articles/misc/Bullet_Graph_Design_Spec.pdf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code changes for threading through the theme are great!
The dark mode outputs would need some joint design work, there's automatic inversion of color (eg. the actual goes from black to white) and it generally doesn't look good, even if the technical capability is there. I can't decide if fixing the actual dark mode colors is better in this PR or a subsequent one.
The bullet graph looks especially non-standard with the inverted colors. It is not the case that color gradients necessarily need to be reversed for dark mode to be effective, it depends (there are some papers that delved into this)
Here are some more artistic yet dark mode friendly versions for bullet graphs. Looks like they originate from here.
Instead of inverting the color gradient, some automatism can probably be achieved via thinking of it as incremental, overlaid bands with some transparency (translucent black if the background is white and the other way around), though in implementation, it's possible to compute this for alpha=1 equivalents.
The tick labels are too faint, if it's the same as Cartesians, it should be reconsidered there too, or maybe goal uses a less heavy font weight because the currently shown tick labels are not sufficiently readable
I didn't immediately see dark mode mocks, so if there's regression with dark mode specifically, is there a way to capture it? |
True, however the design team is separate because their theme removes ticks altogether on cartesian charts. This should just concern us and how we want the charts to look internally, then the design team can adjust their theme accordingly. I simply wanted to match the theme styles across similar chart components.
I personally think we can open a PR up in EUI to discuss how we all expect the goal charts to be themed, then apply any change to our theme if necessary.
I am assuming the user provides some non-default band coloring in which case the default color for the target and performance lines will not always be ideal. We could enhance this by running contrast change to find a better color but even then one band may be better than the other in which case the chart consumer should decide case-by-case now that this line color is configurable. Another strategy could be to use a
Yeah but this is an implementation detail, no?
Thanks, added in 28aa946 |
@monfera updated with discussed changes if you want another look. No rush. |
Summary
Adds dark mode to goal charts with configurable theme styles.
Deprecations (future breaking changes)
The
GoalSpec.config
prop in now deprecated to be removed in future release. All properties have been moved/renamed under newTheme.goal
options with the following exceptions:Config.margin
is now controlled byTheme.chartMargins
and is no longer a margin ratio as before.Config.backgroundColor
is now controlled byTheme.background.color
, even though it's not yet used.fontFamily
moved into each respective label stylesangleStart
andangleEnd
are moved onto theGoalSpec
as optional values.sectorLineWidth
,width
andheight
all removed as they were never used.Details
The
Goal
,Partition
andWordCloud
specs all have no connection to the chart theme and thus have no canonical inheritance to dark mode. This PR fixes this for theGoal
chart and adds some added styles to the text labels as exposed in xy charts. TheConfig
was broken down and moved into theme or the spec accordingly. We should avoid having aConfig
option to control theme as this not only creates a disconnect but also allows for multiple sources of truth for similar options and another property requirement to toggle for theming between dark and light modes.In the future I think it would great if we have a
Config
alternative to the react props in addition to theTheme
options.Issues
closes #1243
Checklist
packages/charts/src/index.ts
(and stories only import from../src
except for test data & storybook)