feat(heatmap): dark mode with theme controls by nickofthyme · Pull Request #1406 · elastic/elastic-charts

nickofthyme · 2021-09-23T22:15:20Z

Summary

Adds dark mode to goal charts with configurable theme styles.

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

Applies #1356 to master

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

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.

packages/charts/src/chart_types/heatmap/state/selectors/get_x_axis_right_overflow.ts

packages/charts/src/chart_types/heatmap/state/selectors/is_brush_available.ts

packages/charts/src/chart_types/heatmap/state/selectors/on_brush_end_caller.ts

monfera · 2021-09-29T16:02:02Z

packages/charts/src/utils/themes/dark_theme.ts

+  heatmap: {
+    maxRowHeight: 30,
+    maxColumnWidth: 30,
+    brushArea: {
+      visible: true,
+      stroke: '#D3DAE6', // euiColorLightShade,


At some point in the future, the longwinded theme object ladders could be farmed out so they're external to src. Their types are already public for external themeability. Worth trying to place them outside src to see if anything balks

monfera

The code looks good to me! I'd rely on @rshen91 or @markov00 doing proper smoke testing, due to more familiarity with themes. I made only optional code comments

The dark mocks look unusual in that (likely) low values are very salient white against the black background, another palette would be more appropriate, so our examples look more or less OK.

For the record I'm not a big fan of the gridline-free mock where it's hard to read which row a cell is in.

Also in some next PR the highlight could be fixed (I discussed it with @rshen91 earlier)

markov00

changes LGTM
We should align and work together with EUI team to introduce the right changes to the theme

packages/charts/src/chart_types/heatmap/state/selectors/get_x_axis_right_overflow.ts

markov00 · 2021-10-05T10:35:25Z

packages/charts/api/charts.api.md

+// Warning: (ae-forgotten-export) The symbol "SpecRequiredProps" needs to be exported by the entry point index.d.ts
+// Warning: (ae-forgotten-export) The symbol "SpecOptionalProps" needs to be exported by the entry point index.d.ts
+//
 // @alpha (undocumented)
-export const Heatmap: React_2.FunctionComponent<Pick<HeatmapSpec, 'id' | 'data' | 'colorScale'> & Partial<Omit<HeatmapSpec, 'chartType' | 'specType' | 'id' | 'data'>>>;
+export const Heatmap: React_2.FunctionComponent<SpecRequiredProps_8 & SpecOptionalProps_8>;


not in this PR but we should find a way to express this in the right way

Could you elaborate what you mean here? Would #1421 solve this?

BREAKING CHANGES: The `WorkcloudSpec.config` prop is removed as it was not used other than assigning `margins` even with erroneous properties. All wordcloud properties are now driven from the `WorkcloudSpec` directly. Since the wordcloud is unique in that it's styles are driven by the data I think keeping them on the spec is more favorable than moving them to the theme as they would be overridden more frequently. This does not provide a themed instance of the chart type but this could possibly come from `.brightening` the provided colors of the text elements.

…eming

# [41.0.0](v40.2.0...v41.0.0) (2021-12-17) ### Bug Fixes * replace createRef with useRef in Functional Components. ([#1524](#1524)) ([9538417](9538417)) ### Code Refactoring * **goal:** remove deprecated config ([#1408](#1408)) ([312e31d](312e31d)) ### Features * **heatmap:** dark mode with theme controls ([#1406](#1406)) ([f29c8dd](f29c8dd)) * **legend:** custom legend width ([#1467](#1467)) ([51f50df](51f50df)) ### BREAKING CHANGES * **goal:** The `GoalSpec.config` prop is removed. 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.

nickofthyme added enhancement New feature or request :styling Styling related issue :heatmap Heatmap/Swimlane chart related issue :theme breaking change labels Sep 23, 2021

nickofthyme requested review from markov00 and monfera September 29, 2021 01:55

nickofthyme added 2 commits September 29, 2021 08:43

Merge branch 'master' into heatmap-theming

38550e5

chore: fix types and test errors from merge

7768e61

monfera reviewed Sep 29, 2021

View reviewed changes

packages/charts/src/chart_types/heatmap/state/selectors/get_x_axis_right_overflow.ts Outdated Show resolved Hide resolved

monfera reviewed Sep 29, 2021

View reviewed changes

packages/charts/src/chart_types/heatmap/state/selectors/is_brush_available.ts Outdated Show resolved Hide resolved

monfera reviewed Sep 29, 2021

View reviewed changes

packages/charts/src/chart_types/heatmap/state/selectors/on_brush_end_caller.ts Show resolved Hide resolved

monfera reviewed Sep 29, 2021

View reviewed changes

markov00 approved these changes Oct 5, 2021

View reviewed changes

nickofthyme added 4 commits November 9, 2021 12:58

Merge branch 'master' into heatmap-theming

73f0985

fix: type errors from merge

9a9639e

chore: update vrt screenshots

3d0c91c

fix: api type errors

f489377

rshen91 mentioned this pull request Nov 24, 2021

feat(heatmap): add axis titles #1503

Merged

5 tasks

nickofthyme and others added 7 commits December 15, 2021 16:21

Merge branch 'master' into heatmap-theming

9df89d6

fix: merge type error

ffb20aa

chore: address comments and failing checks

06240f4

Merge remote-tracking branch 'origin/heatmap-theming' into heatmap-th…

4cd88b4

…eming

fix: api changes

023c563

test: fix vrt theme param interpolation

879ae5d

nickofthyme changed the title ~~feat(heatmap): dark mode with theme controls (#1356)~~ feat(heatmap): dark mode with theme controls Dec 15, 2021

nickofthyme enabled auto-merge (squash) December 15, 2021 23:29

nickofthyme merged commit f29c8dd into elastic:master Dec 15, 2021

nickofthyme deleted the heatmap-theming branch December 15, 2021 23:46

markov00 mentioned this pull request Jan 14, 2022

fix(heatmap): labels visibility regression #1549

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(heatmap): dark mode with theme controls#1406

feat(heatmap): dark mode with theme controls#1406
nickofthyme merged 14 commits intoelastic:masterfrom
nickofthyme:heatmap-theming

nickofthyme commented Sep 23, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

monfera Sep 29, 2021 •

edited

Loading

Uh oh!

monfera left a comment

Uh oh!

markov00 left a comment

Uh oh!

Uh oh!

markov00 Oct 5, 2021

Uh oh!

nickofthyme Dec 15, 2021 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

nickofthyme commented Sep 23, 2021

Summary

BREAKING CHANGES

Details

Issues

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

monfera Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monfera left a comment

Choose a reason for hiding this comment

Uh oh!

markov00 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

markov00 Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

nickofthyme Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

monfera Sep 29, 2021 •

edited

Loading

nickofthyme Dec 15, 2021 •

edited

Loading