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(heatmap): dark mode with theme controls #1356

Merged
merged 6 commits into from
Sep 8, 2021

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Sep 2, 2021

Summary

Adds dark mode to goal charts with configurable theme styles.

Screen Recording 2021-09-02 at 02 31 07 PM

BREAKING CHANGES

The HeatmapSpec.config prop is removed. All properties have been moved/renamed under new Theme.heatmap options with the following exceptions:

  • HeatmapSpec.margin was not used, Theme.chartMargins should be used in the future.
  • HeatmapSpec.fontFamily HeatmapSpec.width and HeatmapSpec.height all removed as they were never used.

Details

The Heatmap, 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 Heatmap chart and adds some added styles to the text labels as exposed in xy charts. The HeatmapConfig was broken down and moved into Theme or the HeatmapSpec 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 #1335

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • The :theme label has been added and the @elastic/eui-design team has been pinged when there are Theme API changes
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • New public API exports have been added to packages/charts/src/index.ts
  • Unit tests have been added or updated to match the most common scenarios
  • The proper documentation and/or storybook story has been added or updated
  • The code has been checked for cross-browser compatibility (Chrome, Firefox, Safari, Edge)
  • Visual changes have been tested with all available themes including dark, light, eui-dark & eui-light

@nickofthyme nickofthyme added enhancement New feature or request :styling Styling related issue :heatmap Heatmap/Swimlane chart related issue :theme labels Sep 2, 2021
@nickofthyme nickofthyme changed the title Heatmap theming feat(heatmap): dark mode with theme controls Sep 2, 2021
@nickofthyme nickofthyme changed the base branch from alpha to master September 2, 2021 19:49
@nickofthyme nickofthyme changed the base branch from master to alpha September 2, 2021 19:49
@nickofthyme nickofthyme marked this pull request as ready for review September 3, 2021 13:58
@nickofthyme nickofthyme requested a review from markov00 September 3, 2021 13:58
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see a visible difference on the echHighlighterMask in this PR in dark mode:
the Y axis and the selected areas are not transparent anymore as in the old implementation:
This PR:
Screenshot 2021-09-06 at 12 21 09
Current master:
Screenshot 2021-09-06 at 12 30 14

Despite the black text, the behaviour on master is the correct one. clearly visible also in the light theme
Screenshot 2021-09-06 at 12 21 02

I'd also like to chat about when a style needs to be pushed into the theme and when it needs to be local on the spec.

packages/charts/src/utils/themes/dark_theme.ts Outdated Show resolved Hide resolved
packages/charts/src/utils/themes/theme.ts Outdated Show resolved Hide resolved
@markov00
Copy link
Member

markov00 commented Sep 6, 2021

just remember that the heatmap is in @alpha and we can also avoid having BREAKING CHANGES there

@nickofthyme
Copy link
Collaborator Author

I'd also like to chat about when a style needs to be pushed into the theme and when it needs to be local on the spec.

Aside from what we discussed in todays meeting, I tend to through all function thing like formatters in the spec rather than the theme.

@nickofthyme nickofthyme requested a review from markov00 September 7, 2021 20:23
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me
tested locally and all the issues are now gone!

@nickofthyme nickofthyme merged commit 4e49150 into elastic:alpha Sep 8, 2021
@nickofthyme nickofthyme deleted the heatmap-theming branch September 8, 2021 14:02
nickofthyme added a commit to nickofthyme/elastic-charts that referenced this pull request Sep 23, 2021
adds dark mode to goal charts with configurable theme styles.

BREAKING CHANGE:

The `HeatmapSpec.config` prop is removed. All properties have been moved/renamed under new `Theme.heatmap` options with the following exceptions:

- `HeatmapSpec.margin` was not used, `Theme.chartMargins` should be used in the future.
- `HeatmapSpec.fontFamily` `HeatmapSpec.width` and `HeatmapSpec.height` all removed as they were never used.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request :heatmap Heatmap/Swimlane chart related issue :styling Styling related issue :theme
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants