feat(bar_chart): add shadow prop for value labels #785

dej611 · 2020-08-19T09:59:50Z

Summary

This is just a small PoC that came up while talking with @markov00 about value labels readability.
This PR introduces a new "stroke" level around the value labels in order to increase the contrast with the bar colour: this is particular efficient when the value text color is white and the stroke is some dark color. Using a darker color for the text fill does not provide the same quality result.

There are still some open questions, like:

would it make sense to dynamically assign the stroke color based on the fill color by default?
- I made some experiments here as well, using brightness to measure the appropriate contrast, and results are not great. But maybe there's a better heuristic.
Should transparent be the default value? I think so, but maybe there's a better idea.
Should the name of the feature be shadow or something else? I started with a box shadow technique to start with, hence the name shadow. But now it is probably something different.

Checklist

Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
This was checked for cross-browser compatibility
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

wylieconlon · 2020-08-20T20:50:23Z

I would consider increasing the shadow size, or making it configurable.

dej611 · 2020-08-24T12:24:37Z

I would consider increasing the shadow size, or making it configurable.

Technically is possible, but I would leave it as last resort.
I'm curious to check if a "golden" value may exist here - not saying the current is the right one. There's some margin for experimenting here.

markov00 · 2020-08-24T13:30:36Z

@dej611 we should get some feedback from @miukimiu here as a design eye can be very useful.
I think this can be useful when the text label overflows the bar edges to avoid having white text on a white background or the opposite, but probably we should keep it pure, without shadow, in the case the text is fully contained within the bar

miukimiu

Ideally, we should by default set the value color to white or black based on the contrast of the bar fill. And always ensure a shadow to increase the readability. Like this example from highcharts.

But if it's not possible right now, I would just improve the example by changing the value labels to white, add a shadow and increase the font-size.

miukimiu · 2020-09-28T11:42:23Z

stories/bar/2_label_value.tsx

@@ -61,6 +61,7 @@ export const Example = () => {
        fontStyle: 'normal',
        padding: 0,
        fill: color('value color', '#000'),


I've been testing and I think for the example white fill color looks better with a dark shadow:

Suggested change

fill: color('value color', '#000'),

fill: color('value color', '#fff'),

I also increased the size of the value font size to 12.

Do you think it should be a default setting or just a better starting point for this example?

I was seeing the other charts where we have labels/values on top and normally they are black or with textInvertible: true

So for consistency, I think we should keep the value labels black and just increase the size to 12.

How do you feel of then creating a second example to show that the value label can be customized? In this new example, you would use white value labels with a shadow.

@miukimiu which would you prefer?

Changing color contrast of the shadow

Changing color contrast of the text itself

Either of the above depending on if a shadow is enabled

From my understanding when it flips the color on text the shadow should change too.
But you are right, there is also the additional scenario where the shadow is not enabled (transparent?) and only the text changes.

Yeah thats true but I was actually thinking that the shadow be null or undefined, as it's not needed when the text contrast is dynamic.

miukimiu · 2020-09-28T11:44:03Z

stories/bar/2_label_value.tsx

@@ -61,6 +61,7 @@ export const Example = () => {
        fontStyle: 'normal',
        padding: 0,
        fill: color('value color', '#000'),
+        shadowColor: color('shadow color', 'transparent'),


I think we should enforce the use of a shadow:

Suggested change

shadowColor: color('shadow color', 'transparent'),

shadowColor: color('shadow color', 'rgba(0,0,0,1)'),

by default in the chart config or just on this one story?

Forgotten to report here, just the story.

dej611 · 2020-10-07T13:30:42Z

the fill property for the DisplayValueStyle handles 3 different scenarios: font color, font + border color, automatic color assignment (w/o border)
A new story has been dedicated completely to the advanced settings for value labels
The second scenario leaves full control to the user:
the third scenario lets the library pick the color:

Note: when the automatic flag is tuned on, the border color has a little bit of alpha blending as from some manual testing works better in general. It's one variable so it's easy to tweak

codecov-io · 2020-10-07T14:03:06Z

Codecov Report

Merging #785 into master will decrease coverage by 0.03%.
The diff coverage is 23.52%.

@@            Coverage Diff             @@
##           master     #785      +/-   ##
==========================================
- Coverage   70.01%   69.97%   -0.04%     
==========================================
  Files         321      336      +15     
  Lines       10508    10198     -310     
  Branches     2221     2035     -186     
==========================================
- Hits         7357     7136     -221     
+ Misses       3142     2995     -147     
- Partials        9       67      +58

Flag	Coverage Δ
#unittests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
..._types/xy_chart/renderer/canvas/primitives/text.ts	`7.21% <0.00%> (-0.48%)`	⬇️
src/utils/themes/theme.ts	`100.00% <ø> (ø)`
...chart_types/xy_chart/renderer/canvas/values/bar.ts	`16.36% <29.62%> (+5.25%)`	⬆️
src/utils/dimensions.ts	`71.42% <0.00%> (-28.58%)`	⬇️
src/utils/data_generators/data_generator.ts	`45.45% <0.00%> (-9.10%)`	⬇️
...rc/chart_types/xy_chart/renderer/canvas/bubbles.ts	`64.70% <0.00%> (-7.52%)`	⬇️
src/components/portal/utils.ts	`21.42% <0.00%> (-7.15%)`	⬇️
.../partition_chart/state/selectors/compute_legend.ts	`79.06% <0.00%> (-5.72%)`	⬇️
...c/chart_types/xy_chart/annotations/rect/tooltip.ts	`89.47% <0.00%> (-5.53%)`	⬇️
...state/selectors/get_internal_is_tooltip_visible.ts	`75.00% <0.00%> (-5.00%)`	⬇️
... and 183 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66e1311...bf9b841. Read the comment docs.

miukimiu

Tested locally and LGTM! 🎉

The shadow definitely improves the readability and it makes it possible to read value labels when a bar is too small in height.

markov00

LGTM, tested locally.
I've just wrote a few comments with small improvements.
I think we can also reduce the scope of the storybook story to a smaller dataset and a smaller number of options or highlighting them on a different tab

markov00 · 2020-10-12T11:18:49Z

api/charts.api.md

+        textInvertible: boolean;
+        textContrast?: number | boolean;
+        textBorder?: boolean;
+    };


It would be great to add also the border width as a customizable option. This can help create different themes and adjust the theme when consuming the chart

markov00 · 2020-10-12T11:24:59Z

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

+        ctx.lineWidth = 1.5;
+        ctx.strokeStyle = font.shadow;
+        ctx.strokeText(text, origin.x, origin.y);
+        ctx.shadowBlur = 0;


shadowBlur is by default 0, probably you want to disable the shadowBlur if configured in the context somewhere else and probably before writing the stroke, so moving this before the strokeText call or before the if statement you want to apply that reset also to the fillText

markov00 · 2020-10-12T11:28:53Z

src/utils/themes/theme.ts

+export type DisplayValueStyle = Omit<TextStyle, 'fill'> & {
  offsetX: number;
  offsetY: number;
+  fill:
+    | Color
+    | { color: Color; borderColor?: Color }
+    | {
+        textInvertible: boolean;
+        textContrast?: number | boolean;
+        textBorder?: boolean;
+      };


This probably is a breaking change that needs to be notified

markov00 · 2020-10-12T11:31:40Z

stories/bar/bars.stories.tsx

@@ -35,6 +35,7 @@ export { Example as withLinearXAxisNoLinearInterval } from './6_linear_no_linear
 export { Example as withTimeXAxis } from './7_with_time_xaxis';
 export { Example as withLogYAxis } from './8_with_log_yaxis';
 export { Example as withStackedLogYAxis } from './9_with_stacked_log';
+export { Example as withValueLabelAdvanced } from './50_label_value_advanced';


can you move this right after the other withValueLabel so the user can play directly with the advanced settings?

…el-shadow

codecov-commenter · 2020-10-12T16:19:09Z

Codecov Report

Merging #785 into master will decrease coverage by 0.03%.
The diff coverage is 23.52%.

@@            Coverage Diff             @@
##           master     #785      +/-   ##
==========================================
- Coverage   70.01%   69.97%   -0.04%     
==========================================
  Files         321      336      +15     
  Lines       10508    10198     -310     
  Branches     2221     2035     -186     
==========================================
- Hits         7357     7136     -221     
+ Misses       3142     2995     -147     
- Partials        9       67      +58

Flag	Coverage Δ
#unittests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
..._types/xy_chart/renderer/canvas/primitives/text.ts	`7.21% <0.00%> (-0.48%)`	⬇️
src/utils/themes/theme.ts	`100.00% <ø> (ø)`
...chart_types/xy_chart/renderer/canvas/values/bar.ts	`16.36% <29.62%> (+5.25%)`	⬆️
src/utils/dimensions.ts	`71.42% <0.00%> (-28.58%)`	⬇️
src/utils/data_generators/data_generator.ts	`45.45% <0.00%> (-9.10%)`	⬇️
...rc/chart_types/xy_chart/renderer/canvas/bubbles.ts	`64.70% <0.00%> (-7.52%)`	⬇️
src/components/portal/utils.ts	`21.42% <0.00%> (-7.15%)`	⬇️
.../partition_chart/state/selectors/compute_legend.ts	`79.06% <0.00%> (-5.72%)`	⬇️
...c/chart_types/xy_chart/annotations/rect/tooltip.ts	`89.47% <0.00%> (-5.53%)`	⬇️
...state/selectors/get_internal_is_tooltip_visible.ts	`75.00% <0.00%> (-5.00%)`	⬇️
... and 183 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66e1311...bf9b841. Read the comment docs.

codecov-io · 2020-10-12T18:26:47Z

Codecov Report

Merging #785 into master will increase coverage by 0.32%.
The diff coverage is 20.51%.

@@            Coverage Diff             @@
##           master     #785      +/-   ##
==========================================
+ Coverage   69.76%   70.08%   +0.32%     
==========================================
  Files         321      336      +15     
  Lines       10566    10889     +323     
  Branches     2171     2211      +40     
==========================================
+ Hits         7371     7632     +261     
- Misses       3186     3242      +56     
- Partials        9       15       +6

Flag	Coverage Δ
#unittests	`69.58% <20.51%> (-0.19%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
..._types/xy_chart/renderer/canvas/primitives/text.ts	`7.14% <0.00%> (-0.55%)`	⬇️
src/utils/themes/theme.ts	`100.00% <ø> (ø)`
...chart_types/xy_chart/renderer/canvas/values/bar.ts	`11.69% <25.00%> (+3.06%)`	⬆️
src/mocks/index.ts	`100.00% <0.00%> (ø)`
src/mocks/specs/index.ts	`100.00% <0.00%> (ø)`
src/mocks/theme.ts	`100.00% <0.00%> (ø)`
src/mocks/specs/specs.ts	`82.75% <0.00%> (ø)`
src/mocks/series/series.ts	`88.13% <0.00%> (ø)`
src/mocks/series/index.ts	`100.00% <0.00%> (ø)`
src/mocks/series/data.ts	`100.00% <0.00%> (ø)`
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 363aeb4...5777029. Read the comment docs.

wylieconlon · 2020-10-12T19:36:35Z

stories/bar/50_label_value_advanced.tsx

+  };
+
+  const debug = boolean('debug', false);
+  const useInverted = boolean('textInverted', false);


I was a bit surprised that the inverted colors aren't configurable in any way, was this intentional?

Yes. There are basically three configurations available for DisplayValueLabels text:

// basic coloring Color | // advanced coloring { color: Color; borderColor?: Color; borderWidth?: number; } | // let the library compute it { textInvertible: boolean; textContrast?: number | boolean; textBorder?: number | boolean; };

The only way to influence the library color pick in case of automatic color assignment is the textContrast parameter, which influence the threshold for the automatic switch between black and white.
This is consistent with other DisplayValueLabels APIs like the partitions one.

…el-shadow

# [24.0.0](v23.2.1...v24.0.0) (2020-10-19) ### Bug Fixes * **annotation:** annotation rendering with no yDomain or groupId ([#842](#842)) ([f173b49](f173b49)), closes [#438](#438) [#798](#798) ### Features * **bar_chart:** add Alignment offset to value labels ([#784](#784)) ([363aeb4](363aeb4)) * **bar_chart:** add shadow prop for value labels ([#785](#785)) ([9b29392](9b29392)) * **bar_chart:** scaled font size for value labels ([#789](#789)) ([3bdd1ee](3bdd1ee)), closes [#788](#788) * **heatmap:** allow fixed right margin ([#873](#873)) ([16cf73c](16cf73c)) ### BREAKING CHANGES * **bar_chart:** The `DisplayValueStyle` `fontSize` property can now express an upper and lower bound as size, used for the automatic scaling. * **bar_chart:** The `DisplayValueStyle` `fill` property can now express a border color and width, or let the library pick the best match based on contrast using the textInvertible parameter.

markov00 · 2020-10-19T13:49:30Z

🎉 This PR is included in version 24.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [24.0.0](elastic/elastic-charts@v23.2.1...v24.0.0) (2020-10-19) ### Bug Fixes * **annotation:** annotation rendering with no yDomain or groupId ([opensearch-project#842](elastic/elastic-charts#842)) ([6bad0d7](elastic/elastic-charts@6bad0d7)), closes [opensearch-project#438](elastic/elastic-charts#438) [opensearch-project#798](elastic/elastic-charts#798) ### Features * **bar_chart:** add Alignment offset to value labels ([opensearch-project#784](elastic/elastic-charts#784)) ([106d924](elastic/elastic-charts@106d924)) * **bar_chart:** add shadow prop for value labels ([opensearch-project#785](elastic/elastic-charts#785)) ([de95b44](elastic/elastic-charts@de95b44)) * **bar_chart:** scaled font size for value labels ([opensearch-project#789](elastic/elastic-charts#789)) ([8b74a9d](elastic/elastic-charts@8b74a9d)), closes [opensearch-project#788](elastic/elastic-charts#788) * **heatmap:** allow fixed right margin ([opensearch-project#873](elastic/elastic-charts#873)) ([dd34574](elastic/elastic-charts@dd34574)) ### BREAKING CHANGES * **bar_chart:** The `DisplayValueStyle` `fontSize` property can now express an upper and lower bound as size, used for the automatic scaling. * **bar_chart:** The `DisplayValueStyle` `fill` property can now express a border color and width, or let the library pick the best match based on contrast using the textInvertible parameter.

feat(bar_chart): add shadow prop for value labels

9c0d1be

dej611 mentioned this pull request Aug 19, 2020

[Lens] New value labels config option for bar charts elastic/kibana#75426

Closed

14 tasks

dej611 self-assigned this Sep 23, 2020

dej611 requested review from markov00 and miukimiu September 23, 2020 13:11

markov00 requested a review from nickofthyme September 24, 2020 16:41

docs: sync API with new labels shadow feature

f2d0f69

dej611 marked this pull request as ready for review September 28, 2020 08:29

miukimiu suggested changes Sep 28, 2020

View reviewed changes

dej611 added 2 commits October 7, 2020 15:24

feat: implement textInverted logic for value labels

7051c92

chore: remove shadow reference in demo

8570d67

dej611 requested a review from miukimiu October 7, 2020 13:48

docs: sync new API for fill value

e42ad56

test: add missing test snapshot

0499acc

miukimiu approved these changes Oct 7, 2020

View reviewed changes

markov00 approved these changes Oct 12, 2020

View reviewed changes

dej611 added 3 commits October 12, 2020 17:19

feat: implemented various suggestions from review

14a26f2

Merge remote-tracking branch 'upstream/master' into feature/value-lab…

0035949

…el-shadow

docs: updated api changes

bf9b841

test: remade snapshot image

5857623

wylieconlon reviewed Oct 12, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into feature/value-lab…

86dad5d

…el-shadow

dej611 added 2 commits October 13, 2020 15:49

refactor: armonize with latest master and alignment pr

58a5651

test: fixing visual snapshots for new features

5777029

dej611 merged commit 9b29392 into elastic:master Oct 13, 2020

dej611 deleted the feature/value-label-shadow branch October 13, 2020 16:55

markov00 added the released Issue released publicly label Oct 19, 2020

markov00 mentioned this pull request Nov 3, 2020

Improve defaults for value labels border size #886

Open

nickofthyme mentioned this pull request Jun 7, 2021

Change value labels colors depending on background #238

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(bar_chart): add shadow prop for value labels #785

feat(bar_chart): add shadow prop for value labels #785

dej611 commented Aug 19, 2020 •

edited

Loading

wylieconlon commented Aug 20, 2020

dej611 commented Aug 24, 2020

markov00 commented Aug 24, 2020

miukimiu left a comment •

edited

Loading

miukimiu Sep 28, 2020 •

edited

Loading

miukimiu Sep 28, 2020 •

edited

Loading

dej611 Sep 28, 2020

miukimiu Sep 28, 2020

nickofthyme Sep 29, 2020

dej611 Sep 29, 2020

nickofthyme Sep 29, 2020

miukimiu Sep 28, 2020 •

edited

Loading

nickofthyme Sep 29, 2020

dej611 Sep 29, 2020

dej611 commented Oct 7, 2020 •

edited

Loading

codecov-io commented Oct 7, 2020 •

edited

Loading

miukimiu left a comment

markov00 left a comment

markov00 Oct 12, 2020

markov00 Oct 12, 2020

markov00 Oct 12, 2020

markov00 Oct 12, 2020

codecov-commenter commented Oct 12, 2020

codecov-io commented Oct 12, 2020 •

edited

Loading

wylieconlon Oct 12, 2020

dej611 Oct 13, 2020 •

edited

Loading

markov00 commented Oct 19, 2020

	fill: color('value color', '#000'),
	fill: color('value color', '#fff'),

	shadowColor: color('shadow color', 'transparent'),
	shadowColor: color('shadow color', 'rgba(0,0,0,1)'),

feat(bar_chart): add shadow prop for value labels #785

feat(bar_chart): add shadow prop for value labels #785

Conversation

dej611 commented Aug 19, 2020 • edited Loading

Summary

Checklist

wylieconlon commented Aug 20, 2020

dej611 commented Aug 24, 2020

markov00 commented Aug 24, 2020

miukimiu left a comment • edited Loading

Choose a reason for hiding this comment

miukimiu Sep 28, 2020 • edited Loading

Choose a reason for hiding this comment

miukimiu Sep 28, 2020 • 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

miukimiu Sep 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dej611 commented Oct 7, 2020 • edited Loading

codecov-io commented Oct 7, 2020 • edited Loading

Codecov Report

miukimiu left a comment

Choose a reason for hiding this comment

markov00 left a 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

codecov-commenter commented Oct 12, 2020

Codecov Report

codecov-io commented Oct 12, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

dej611 Oct 13, 2020 • edited Loading

Choose a reason for hiding this comment

markov00 commented Oct 19, 2020

dej611 commented Aug 19, 2020 •

edited

Loading

miukimiu left a comment •

edited

Loading

miukimiu Sep 28, 2020 •

edited

Loading

miukimiu Sep 28, 2020 •

edited

Loading

miukimiu Sep 28, 2020 •

edited

Loading

dej611 commented Oct 7, 2020 •

edited

Loading

codecov-io commented Oct 7, 2020 •

edited

Loading

codecov-io commented Oct 12, 2020 •

edited

Loading

dej611 Oct 13, 2020 •

edited

Loading