feat(heatmap): add text color contrast to heatmap cells #1342

rshen91 · 2021-08-30T14:58:33Z

Summary

Text will now take into account the background color of the cell and adjust its contrast to 4.5.

Details

Still to do:

Confirm that true textContrast will default to 4.5
Fix the makeHighContrastColor function for cases where contrast is hard/impossible (commit 2eaa8e6)
Take into account the background color of the chart in addition to the background color (commit 5c6b739)

Issues

Closes #1296

Checklist

The proper chart type label has been added (e.g. :xy, :partition)
The proper feature labels have been added (e.g. :interactions, :axis)
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
The proper documentation and/or storybook story has been added or updated

markov00 · 2021-08-31T16:15:17Z

@rshen91 I've few doubts on your todo list:

Fix bullet chart stories without having contrast defined

we should fix the bullet chart in a different PR not this one. I'm also not sure what you mean by fix stories without having contrast defined.

Take into account the background color of the chart in addition to the background color

Can you please explain to me why? in heatmap we are rendering the text on top of a cell, that is colored. Why we need to consider the background color of the chart? for semi-transparent colors?

rshen91 · 2021-08-31T16:58:36Z

@rshen91

I have some stories locally that look like they're failing these changes but they seem to be fine in vrt testing in master. I'm thinking because they're not failing in master then it's fine?

It looks now they are rendering fine right? merging master probably was the fix

Extra feature: add a way to pass the page background color as well to handle constrast measurement with rgba values (we hit this bug already in table Lens dynamic coloring).

I think we can fix that in a future PR, just open a different issue with that ok?

…ontrast

packages/charts/src/chart_types/heatmap/layout/types/config_types.ts

markov00 · 2021-09-02T09:40:39Z

packages/charts/src/chart_types/heatmap/layout/types/config_types.ts

@@ -87,6 +87,7 @@ export interface Config {
      align: TextAlign;
      baseline: TextBaseline;
      visible: boolean;
+      textContrast: boolean | number;


let's reduce the possibilities there. My personal opinion is that we should always invert the text color if there is not enough contrast.
If you are ok we can remove this configuration option and have the code that always invert by default the color.
If requested, we can then expose a configuration option to turn this off or change the contrast ratio

f32b32d for changes thanks!

packages/charts/src/chart_types/xy_chart/renderer/canvas/primitives/text.ts

markov00 · 2021-09-02T09:49:26Z

packages/charts/src/chart_types/xy_chart/renderer/canvas/primitives/text.ts

 ) {
+  const { textContrast } = font;
+  const convertPageBackgroundColor = getHexValue(pageBackgroundColor);


you should not need an hex value for the fillTextColor method

For sure! So the issue I was facing was using rgba(0,0,0,0) instead of #00000 both pointing to a transparent color. The fillTextColor() has a warning for rgba values with an alpha value of 0. But yes, overall I do think it makes the most sense to have the fillTextColor() just using rgba values.

rgba(0,0,0,0) is transparent
If you want to use black is rgba(0,0,0,1) with full opacity
#0 is black

markov00 · 2021-09-02T09:52:55Z

packages/charts/src/common/color_calcs.ts

+      const contrastColor =
+        originalhighContrastTextColor === 'rgba(255, 255, 255, 1)' || originalhighContrastTextColor === '#fff'
+          ? '#000'
+          : '#fff';
+      // make sure the new text color hits the ratio, if not, then return the scaledContrast since we tried earlier
+      return getContrast(contrastColor, background) > ratio ? contrastColor : scaledContrast.toString();


two comments here:
1- can you explain why you have made these changes?
2- instead of use both hex and rgba values, can you please always use rgba strings instead where possible?

I was noticing one of the cell background colors (rgba(44, 127, 184, 1), #2c7fb8) was going to white text with worse contrast than if it was black text. I hunted the issue down to this edge case and added this to catch this issue in the future.

I agree, I will make sure to refactor to use rgba values where possible instead of hex. I got hung up on the transparent hex value being passed by default but I agree that the rgba value makes the most sense. Will include this in my refactoring, thanks!

f32b32d for changes thank you

markov00 · 2021-09-02T09:53:49Z

packages/charts/src/common/color_library_wrappers.ts

+export function getHexValue(color: Color) {
+  return RGBAToHex(RGBtoString(getColor(color)));
+}
+


isn't that function the same as RGBAToHex?

Refactored in f32b32d thank you!

it looks like the function still there

removed in 0a9a914 sorry!

rshen91 · 2021-09-02T16:22:41Z

jenkins, test this

markov00 · 2021-09-02T16:32:26Z

packages/charts/src/chart_types/heatmap/layout/types/config_types.ts

@@ -87,6 +86,7 @@ export interface Config {
      align: TextAlign;
      baseline: TextBaseline;
      visible: boolean;
+      textContrast: boolean;


let's remove this for now from the configuration. If requested we can add that configuration, but I prefer to remove it

Ok cool sounds good to me 0a9a914 for changes thanks!

markov00 · 2021-09-02T16:33:26Z

packages/charts/src/chart_types/heatmap/layout/viewmodel/viewmodel.ts

+      textColor: fillTextColor(
+        config.cell.label.textColor,
+        true,
+        config.cell.label.textContrast ? 4.5 : false,


As written on my previous comment, the color inversion should always happen, so you can replace this with always the 4.5 value

Ok I think Im following 0a9a914 thanks!

markov00 · 2021-09-02T17:03:08Z

packages/charts/src/chart_types/heatmap/layout/viewmodel/viewmodel.ts

+        true,
+        config.cell.label.textContrast ? 4.5 : false,
+        color,
+        theme.background.color === 'transparent' ? '#000000' : theme.background.color,


if with this code you mean: if the theme background color is transparent keyword use black as the fallback, I think we need to change that to use white instead, as the default theme is the LIGHT one
Use white rgba if possible rgba(255,255,255,1)

0a9a914 thank you!

markov00 · 2021-09-02T17:04:57Z

packages/charts/src/common/color_calcs.ts

+          ? '#000'
+          : '#fff';


Suggested change

? '#000'

: '#fff';

? 'rgba(0,0,0,1)'

: 'rgba(255,255,255,1)';

Yikes sorry! 0a9a914 thanks

markov00 · 2021-09-02T17:06:01Z

packages/charts/src/common/color_library_wrappers.ts

+export function getHexValue(color: Color) {
+  return RGBAToHex(RGBtoString(getColor(color)));
+}
+


it looks like the function still there

markov00 · 2021-09-02T17:07:35Z

packages/charts/src/common/fill_text_color.ts

-  const containerBackground = combineColors(shapeFillColor, backgroundColor);
+  const containerBackground = combineColors(
+    shapeFillColor,
+    backgroundColor === 'transparent' ? '#000000' : backgroundColor,


similar to a previous comment:
the background fallback for transparent color should be white (because the default theme is the light one)
So if you need to replace the transparent with a valid fallback please use rgba(255,255,255,1)

Good catch thank you 0a9a914

markov00 · 2021-09-02T17:08:33Z

storybook/stories/heatmap/heatmap.stories.tsx

@@ -13,3 +13,4 @@ export default {
 export { Example as basic } from './1_basic.story';
 export { Example as time } from './3_time.story';
 export { Example as categorical } from './2_categorical.story';
+export { Example as textColorContrast } from './4_color_text.story';


instead of creating a new vrt for that, just show by default the value labels on the categorical example,we can save two new tests (please also remove the added test as this replace the need of the vrt added)

I think I did this in 0a9a914 thank you

markov00

I'm ok merging this PR for now.
I will take a look and clean up the color contrast functions in a next PR because it doesn't look very clean and readable.

# [35.0.0](v34.2.1...v35.0.0) (2021-09-13) ### Bug Fixes * **a11y:** restore focus after popover close with color picker ([#1272](#1272)) ([0c6f945](0c6f945)), closes [#1266](#1266) [#935](#935) * **build:** fix license in package.json ([#1362](#1362)) ([d524fdf](d524fdf)) * **deps:** update dependency @elastic/eui to ^37.5.0 ([#1341](#1341)) ([fb05c98](fb05c98)) * **deps:** update dependency @elastic/eui to ^37.6.1 ([#1359](#1359)) ([2ae90ce](2ae90ce)) * **deps:** update dependency @elastic/eui to ^37.7.0 ([#1373](#1373)) ([553b6b0](553b6b0)) * **heatmap:** filter out tooltip picked shapes in x-axis area ([#1351](#1351)) ([174047d](174047d)), closes [#1215](#1215) * **heatmap:** remove values when brushing only over axes ([#1364](#1364)) ([77ff8d3](77ff8d3)) ### Features * **annotations:** add onClickHandler for annotations ([#1293](#1293)) ([48198be](48198be)), closes [#1211](#1211) * **heatmap:** add text color contrast to heatmap cells ([#1342](#1342)) ([f9a26ef](f9a26ef)), closes [#1296](#1296) * **heatmap:** reduce font size to fit label within cells ([#1352](#1352)) ([16b5546](16b5546)) * **xy:** mutilayer time axis step 1 ([#1326](#1326)) ([867b1f5](867b1f5)) ### BREAKING CHANGES * **xy:** - feat: removes the axis deduplication feature - fix: `showDuplicatedTicks` causes a duplication check on the actual axis tick label (possibly yielded by `Axis.tickLabel` rather than the more general `tickFormat`) * **heatmap:** the `config.label.fontSize` prop is replaced by `config.label.minFontSize` and `config.label.maxFontSize`. You can specify the same value for both properties to have a fixed font size. The `config.label.align` and `config.label.baseline` props are removed from the `HeatmapConfig` object.

nickofthyme · 2021-09-13T15:57:57Z

🎉 This PR is included in version 35.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

rshen91 added 4 commits August 30, 2021 08:58

feat: add text color contrast to heatmap cells

3013ba2

Merge remote-tracking branch 'upstream/master' into heatmap-contrast

af72d13

test: update vrts

c99ae8f

fix: color contrast fix

2eaa8e6

rshen91 added :heatmap Heatmap/Swimlane chart related issue wip work in progress labels Aug 30, 2021

Merge branch 'master' into heatmap-contrast

b2c203d

markov00 added :styling Styling related issue bug Something isn't working labels Sep 1, 2021

rshen91 added 8 commits September 1, 2021 07:36

Merge remote-tracking branch 'upstream/master' into heatmap-contrast

d6a62f2

Merge remote-tracking branch 'origin/heatmap-contrast' into heatmap-c…

7ed0a1d

…ontrast

fix: update heatmap vrts

3d9754e

feat: add pageBackgroundColor for consumers

5c6b739

Merge remote-tracking branch 'upstream/master' into heatmap-contrast

bdb82fb

Merge branch 'master' into heatmap-contrast

5f11829

fix: add pageBackgroundColor input validation check

be11f91

test: add heatmap text color contrast story

3ad46b9

rshen91 marked this pull request as ready for review September 1, 2021 15:44

rshen91 requested review from monfera, nickofthyme and markov00 September 1, 2021 15:44

markov00 requested changes Sep 2, 2021

View reviewed changes

fix: code review changes

f32b32d

markov00 reviewed Sep 2, 2021

View reviewed changes

rshen91 added 2 commits September 2, 2021 11:51

fix: code review part 2

0a9a914

Merge remote-tracking branch 'upstream/master' into heatmap-contrast

122e49e

rshen91 requested a review from markov00 September 2, 2021 17:54

markov00 approved these changes Sep 8, 2021

View reviewed changes

rshen91 added 3 commits September 8, 2021 08:04

Merge remote-tracking branch 'upstream/master' into heatmap-contrast

b0186bd

test: update vrts

c785179

Merge remote-tracking branch 'upstream/master' into heatmap-contrast

c40665b

rshen91 merged commit f9a26ef into elastic:master Sep 8, 2021

rshen91 deleted the heatmap-contrast branch September 8, 2021 14:42

nickofthyme added the released Issue released publicly label Sep 13, 2021

markov00 mentioned this pull request Oct 5, 2021

Update dependency @elastic/charts to v37 (master) elastic/kibana#113968

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(heatmap): add text color contrast to heatmap cells #1342

feat(heatmap): add text color contrast to heatmap cells #1342

rshen91 commented Aug 30, 2021 •

edited

Loading

markov00 commented Aug 31, 2021

rshen91 commented Aug 31, 2021 •

edited by markov00

Loading

markov00 Sep 2, 2021

rshen91 Sep 2, 2021

markov00 Sep 2, 2021

rshen91 Sep 2, 2021

markov00 Sep 2, 2021 •

edited

Loading

markov00 Sep 2, 2021

rshen91 Sep 2, 2021

rshen91 Sep 2, 2021

markov00 Sep 2, 2021

rshen91 Sep 2, 2021

markov00 Sep 2, 2021

rshen91 Sep 2, 2021

rshen91 commented Sep 2, 2021

markov00 Sep 2, 2021

rshen91 Sep 2, 2021

markov00 Sep 2, 2021

rshen91 Sep 2, 2021

markov00 Sep 2, 2021

rshen91 Sep 2, 2021

markov00 Sep 2, 2021

rshen91 Sep 2, 2021

markov00 Sep 2, 2021

markov00 Sep 2, 2021

rshen91 Sep 2, 2021

markov00 Sep 2, 2021

rshen91 Sep 2, 2021

markov00 left a comment

nickofthyme commented Sep 13, 2021

feat(heatmap): add text color contrast to heatmap cells #1342

feat(heatmap): add text color contrast to heatmap cells #1342

Conversation

rshen91 commented Aug 30, 2021 • edited Loading

Summary

Details

Issues

Checklist

markov00 commented Aug 31, 2021

rshen91 commented Aug 31, 2021 • edited by markov00 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markov00 Sep 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rshen91 commented Sep 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markov00 left a comment

Choose a reason for hiding this comment

nickofthyme commented Sep 13, 2021

rshen91 commented Aug 30, 2021 •

edited

Loading

rshen91 commented Aug 31, 2021 •

edited by markov00

Loading

markov00 Sep 2, 2021 •

edited

Loading