Skip to content
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

Merged
merged 18 commits into from Aug 20, 2021

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Aug 14, 2021

Summary

Adds dark mode to goal charts with configurable theme styles.

Screen Recording 2021-08-14 at 10 25 14 AM

Deprecations (future breaking changes)

The GoalSpec.config prop in now deprecated to be removed in future release. All properties have been moved/renamed under new Theme.goal options with the following exceptions:

  • Config.margin is now controlled by Theme.chartMargins and is no longer a margin ratio as before.
  • Config.backgroundColor is now controlled by Theme.background.color, even though it's not yet used.
  • fontFamily moved into each respective label styles
  • angleStart and angleEnd are moved onto the GoalSpec as optional values.
  • sectorLineWidth, width and height all removed as they were never used.

All moved/rename properties will still use the values off the config prop until fully deprecated in a future release.

Details

The Goal, Partition and WordCloud specs all have no connection to the chart theme and thus have no canonical inheritance to dark mode. This PR fixes this for the Goal chart and adds some added styles to the text labels as exposed in xy charts. The Config was broken down and moved into theme or the spec accordingly. We should avoid having a Config 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 the Theme options.

Issues

closes #1243

Checklist

  • The proper chart type label was added (e.g. :xy, :partition) if the PR involves a specific chart type
  • The proper feature label was added (e.g. :interactions, :axis) if the PR involves a specific chart feature
  • Whenever possible, please check if the closing issue is connected to a running GH project
  • Any consumer-facing exports were added to packages/charts/src/index.ts (and stories only import from ../src except for test data & storybook)
  • Proper documentation or storybook story was added for features that require explanation or tutorials
  • Unit tests were updated or added to match the most common scenarios

@nickofthyme nickofthyme added enhancement New feature or request :styling Styling related issue breaking change :goal/gauge (old) Old Goal/Gauge chart related issues labels Aug 14, 2021
@nickofthyme
Copy link
Collaborator Author

@monfera & @markov00 do you think we should use the same colors for the ticks for goal charts as xy charts.

@monfera
Copy link
Contributor

monfera commented Aug 17, 2021

It looks great, Nick!

do you think we should use the same colors for the ticks for goal charts as xy charts

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 /common as one of the shared projections

@nickofthyme nickofthyme marked this pull request as ready for review August 17, 2021 17:13
@monfera
Copy link
Contributor

monfera commented Aug 18, 2021

Nick, big improvement over the original all around 🎉 Looks like the API just needs a couple of pieces of documentation due to policy

@monfera
Copy link
Contributor

monfera commented Aug 18, 2021

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?

image

@monfera
Copy link
Contributor

monfera commented Aug 18, 2021

... @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

image

Copy link
Contributor

@monfera monfera left a 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)

image

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

@monfera
Copy link
Contributor

monfera commented Aug 18, 2021

I didn't immediately see dark mode mocks, so if there's regression with dark mode specifically, is there a way to capture it?

@nickofthyme
Copy link
Collaborator Author

nickofthyme commented Aug 19, 2021

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?

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.

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.

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.

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)

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 chromajs lightening color but in this should not be on by default. Both strategies are best suited for follow-up PR IMHO.

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.

Yeah but this is an implementation detail, no?

I didn't immediately see dark mode mocks, so if there's regression with dark mode specifically, is there a way to capture it?

Thanks, added in 28aa946

@nickofthyme
Copy link
Collaborator Author

@monfera updated with discussed changes if you want another look. No rush.

@nickofthyme nickofthyme enabled auto-merge (squash) August 20, 2021 19:56
auto-merge was automatically disabled August 20, 2021 21:28

Pull request was closed

@nickofthyme nickofthyme reopened this Aug 20, 2021
@nickofthyme nickofthyme reopened this Aug 20, 2021
@elastic elastic deleted a comment from github-actions bot Aug 20, 2021
@elastic elastic deleted a comment from github-actions bot Aug 20, 2021
@nickofthyme nickofthyme merged commit 70873e8 into elastic:master Aug 20, 2021
@nickofthyme nickofthyme deleted the goal-dark-mode branch August 20, 2021 23:57
nickofthyme added a commit to nickofthyme/elastic-charts that referenced this pull request Aug 23, 2021
nickofthyme pushed a commit that referenced this pull request Aug 23, 2021
# [34.2.0](v34.1.1...v34.2.0) (2021-08-23)

### Bug Fixes

* **heatmap:** limit brush tool ([#1270](#1270)) ([509cf42](509cf42)), closes [#1216](#1216)

### Features

* **goal:** dark mode with theme controls ([#1299](#1299)) ([3a583e5](3a583e5))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request :goal/gauge (old) Old Goal/Gauge chart related issues :styling Styling related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Darkmode for gauge/goal
2 participants