feat(bar_chart): scaled font size for value labels #789

wylieconlon · 2020-08-20T20:41:52Z

This is my first attempt at creating scaled font sizes for text in the bar charts, which has similarity to the scaling logic that is implemented in partition charts by @monfera. I've applied the simplest possible logic, which is to take the maximum font size that fits these restrictions:

Count the number of characters in the label
Divide the available bar width by the number of characters, and take the integer value that's smaller
If the font size is less than the minimum in the theme, then use the minimum size instead

It doesn't work correctly for horizontal charts because I don't know how to determine rotation.

Fixes #788

Checklist

Delete any items that are not applicable to this PR.

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

markov00

Thank you @wylieconlon for the PR.
I've added a suggestion to make this work to compute that target font size in a more precise way than the current proposed solution.
If you need the chart rotation parameter, unfortunately is not available at that level, but you can pass it down from computeSeriesGeometries ==> renderGeometries ==> renderBars

I've also a doubt about the readability of the text using this varying-font-size approach:
longer text (due to more decimal points or higher value) can results in smaller font sizes (see the image as an extreme example).

On a data visualization, these font changes can be interpreted in different ways from the users, assuming more importance to bigger texts rather than the smaller ones. Uniform font sizes are preferable if we don't need to communicate any exceptional significance to that value. @monfera @nickofthyme thoughts on that?

src/chart_types/xy_chart/rendering/rendering.ts

wylieconlon · 2020-08-24T14:43:57Z

@markov00 Thanks for the feedback! I agree that uniform font sizes would look much better. To implement your suggestion, I think we would need to calculate the formatted values for all bars, and then scale the font size based on the highest character count of the formatted values. What do you think about that approach?

markov00 · 2020-08-25T08:18:56Z

@markov00 Thanks for the feedback! I agree that uniform font sizes would look much better. To implement your suggestion, I think we would need to calculate the formatted values for all bars, and then scale the font size based on the highest character count of the formatted values. What do you think about that approach?

I think we should compute the bounding box of all the formatted values and then scale the font size based on the maximum width of these boundingboxes.

wylieconlon · 2020-08-25T21:14:13Z

@markov00 Do you think it makes sense to change from fontSize on the API level to minFontSize and maxFontSize?

markov00 · 2020-08-26T08:49:15Z

@markov00 Do you think it makes sense to change from fontSize on the API level to minFontSize and maxFontSize?

@wylieconlon we can have that change without introducing a breaking change if we use a union type that covers two cases: fixed font size or scaled font size. Something like this could work:

export interface TextStyle {
  fontSize: number | { min: number; max: number };
  ...
}

With that, we can handle both use cases: users that want a specific fixed font size + users that look at a scalable font size

Introduce a range size format for bar chart labels to auto scale and improve readability Address elastic#788

dej611 · 2020-09-22T13:07:24Z

Reworked the PR to address the various feedback:

✨ Add a min/max range format for DisplayValueStyle type
- why not TextStyle? Because it has a general effect on the whole code base not affected by the change - breaking many tests as well
✨ Used the scale factor approach to rescale size if a range is passed
- 🐛 tuned isValueContainedInElement to work with the new auto scaling feature
- ✨ tuned the size scaling based on chart rotation
- 💄 Labels now use 70% of space width - value hardcoded
⚠️ small bugfixes for linting warnings

src/chart_types/xy_chart/rendering/rendering.ts

wylieconlon · 2020-09-22T14:43:14Z

src/chart_types/xy_chart/rendering/rendering.ts

+      displayValueSettings,
+    );
+
+    const isHorizontalRotation = chartRotation == null || [0, 180].includes(chartRotation);


Either the logic or naming is wrong, the horizontal charts are 90 and 270: https://elastic.github.io/elastic-charts/?path=/story/rotations--rotations-90-deg-ordinal

Eheh I had a chat with @markov00 about that the other day. He explained to me that their naming reference where the bars are located (for a vertical bar chart they are located on the x axis, therefore horizontal).
I used isVertical here before which makes more sense for me as well, then changed after the chat as consitency is best when maintain the codebase.

Always show labels if enabled but stick min/max values when auto scaling go beyond them

Sync API with the new fontsize format changes for bar charts

codecov-commenter · 2020-09-28T09:23:54Z

Codecov Report

Merging #789 into master will decrease coverage by 0.02%.
The diff coverage is 55.26%.

@@            Coverage Diff             @@
##           master     #789      +/-   ##
==========================================
- Coverage   70.01%   69.98%   -0.03%     
==========================================
  Files         321      336      +15     
  Lines       10508    10189     -319     
  Branches     2221     2031     -190     
==========================================
- Hits         7357     7131     -226     
+ Misses       3142     2991     -151     
- 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.52% <0.00%> (-0.17%)`	⬇️
src/chart_types/xy_chart/state/utils/utils.ts	`92.27% <ø> (-0.01%)`	⬇️
src/mocks/geometries.ts	`86.84% <ø> (ø)`
src/utils/geometry.ts	`100.00% <ø> (ø)`
src/utils/themes/theme.ts	`100.00% <ø> (ø)`
...chart_types/xy_chart/renderer/canvas/values/bar.ts	`12.04% <20.00%> (+0.93%)`	⬆️
src/chart_types/xy_chart/rendering/rendering.ts	`91.11% <74.07%> (-2.18%)`	⬇️
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%)`	⬇️
... and 187 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...7020fa7. Read the comment docs.

markov00

Looks good to me, I've checked the code and tested locally seems to work fine and scales correctly all the values. I've check also the scale with very different value lengths and seems to scale correctly per bar.
I've left a few comments to address (division by zero checks) and some other nit picks

src/chart_types/xy_chart/rendering/rendering.ts

src/chart_types/xy_chart/state/utils/utils.ts

stories/bar/50_simple_value_label.tsx

nickofthyme

Looks amazing so far.

I get an error when enabling both the contain within label and alternating label options.

dej611 · 2020-10-06T17:07:10Z

Yes, that's related to this bug: #786 .

markov00

Tested locally and it looks very good!
Just a small typo on the story title

stories/bar/bars.stories.tsx

Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>

cla-checker-service · 2020-10-12T15:37:10Z

💚 CLA has been signed

wylieconlon

Looking good @dej611! I can't approve since this is technically my own PR, but left some small comments.

wylieconlon · 2020-10-12T19:29:50Z

stories/bar/50_simple_value_label.tsx

+        fill: color('value color', '#000'),
+        offsetX: number('offsetX', 0),
+        offsetY: number('offsetY', 0),
+        alignment: {


The alignment properties don't appear to do anything yet, maybe remove this?

wylieconlon · 2020-10-12T19:30:52Z

stories/bar/50_simple_value_label.tsx

+  h: frozenDataHighVolume,
+};
+
+export const Example = () => {


You've created a new storybook, but I noticed that you didn't update the existing storybook for value labels. I found it a little confusing not to have these options on the "value label" storybook. Would it be worth combining into a single story?

I've left this in here as it will probably be merged with other stories in related PRs to make an Advanced value label story.

codecov-io · 2020-10-13T08:06:26Z

Codecov Report

Merging #789 into master will increase coverage by 0.06%.
The diff coverage is 58.33%.

@@            Coverage Diff             @@
##           master     #789      +/-   ##
==========================================
+ Coverage   69.58%   69.64%   +0.06%     
==========================================
  Files         321      336      +15     
  Lines       10605    10269     -336     
  Branches     2187     2066     -121     
==========================================
- Hits         7379     7152     -227     
+ Misses       3217     3050     -167     
- 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.00% <0.00%> (-0.15%)`	⬇️
src/chart_types/xy_chart/state/utils/utils.ts	`92.27% <ø> (-0.01%)`	⬇️
src/mocks/geometries.ts	`86.84% <ø> (ø)`
src/utils/geometry.ts	`100.00% <ø> (ø)`
src/utils/themes/theme.ts	`100.00% <ø> (ø)`
...chart_types/xy_chart/renderer/canvas/values/bar.ts	`13.60% <50.00%> (+1.90%)`	⬆️
src/chart_types/xy_chart/rendering/rendering.ts	`91.11% <74.07%> (-2.18%)`	⬇️
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%)`	⬇️
... and 187 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 9b29392...07ea3e3. Read the comment docs.

# [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:33Z

🎉 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): scaled font size for value labels

298ee63

wylieconlon requested a review from markov00 August 20, 2020 21:27

markov00 reviewed Aug 24, 2020

View reviewed changes

src/chart_types/xy_chart/rendering/rendering.ts Outdated Show resolved Hide resolved

wylieconlon mentioned this pull request Aug 25, 2020

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

Closed

14 tasks

wylieconlon assigned dej611 Aug 26, 2020

feat(bar-labels): auto size scale for bar labels

4fb764f

Introduce a range size format for bar chart labels to auto scale and improve readability Address elastic#788

wylieconlon commented Sep 22, 2020

View reviewed changes

feat(bar-labels): stick min/max scale if auto value go beyond

c8c91b2

Always show labels if enabled but stick min/max values when auto scaling go beyond them

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

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

docs: sync new API changes

24a651b

Sync API with the new fontsize format changes for bar charts

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

test: fix new snapshot

92c290b

markov00 reviewed Sep 29, 2020

View reviewed changes

dej611 added 3 commits October 6, 2020 17:34

fix: make sure fontSize is always positive

3165ec4

fix: make sure referenceWidth is positive

0ad5731

fix: reordered function args for better dx

b8383bc

nickofthyme reviewed Oct 6, 2020

View reviewed changes

markov00 approved these changes Oct 12, 2020

View reviewed changes

stories/bar/bars.stories.tsx Outdated Show resolved Hide resolved

Update stories/bar/bars.stories.tsx

0d4e56f

Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>

dej611 force-pushed the scaled-value-labels branch from 338fd5d to 0d4e56f Compare October 12, 2020 15:45

Merge remote-tracking branch 'upstream/master' into scaled-value-labels

7020fa7

wylieconlon commented Oct 12, 2020

View reviewed changes

refactor: remove unused settings

fcdce77

dej611 added 4 commits October 13, 2020 15:19

Merge remote-tracking branch 'upstream/master' into scaled-value-labels

1a2f42b

Merge remote-tracking branch 'upstream/master' into scaled-value-labels

3a7f301

fix: unify value stories and fix shadow issues with auto scaling

58a7921

test: algin tests with latest master

07ea3e3

dej611 merged commit 3bdd1ee into elastic:master Oct 14, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(bar_chart): scaled font size for value labels #789

feat(bar_chart): scaled font size for value labels #789

wylieconlon commented Aug 20, 2020

markov00 left a comment •

edited

Loading

wylieconlon commented Aug 24, 2020

markov00 commented Aug 25, 2020

wylieconlon commented Aug 25, 2020

markov00 commented Aug 26, 2020

dej611 commented Sep 22, 2020

wylieconlon Sep 22, 2020

dej611 Sep 22, 2020

codecov-commenter commented Sep 28, 2020 •

edited

Loading

markov00 left a comment •

edited

Loading

nickofthyme left a comment •

edited

Loading

dej611 commented Oct 6, 2020

markov00 left a comment

cla-checker-service bot commented Oct 12, 2020 •

edited

Loading

wylieconlon left a comment

wylieconlon Oct 12, 2020

wylieconlon Oct 12, 2020

dej611 Oct 13, 2020 •

edited

Loading

codecov-io commented Oct 13, 2020 •

edited

Loading

markov00 commented Oct 19, 2020

feat(bar_chart): scaled font size for value labels #789

feat(bar_chart): scaled font size for value labels #789

Conversation

wylieconlon commented Aug 20, 2020

Checklist

markov00 left a comment • edited Loading

Choose a reason for hiding this comment

wylieconlon commented Aug 24, 2020

markov00 commented Aug 25, 2020

wylieconlon commented Aug 25, 2020

markov00 commented Aug 26, 2020

dej611 commented Sep 22, 2020

wylieconlon Sep 22, 2020

Choose a reason for hiding this comment

dej611 Sep 22, 2020

Choose a reason for hiding this comment

codecov-commenter commented Sep 28, 2020 • edited Loading

Codecov Report

markov00 left a comment • edited Loading

Choose a reason for hiding this comment

nickofthyme left a comment • edited Loading

Choose a reason for hiding this comment

dej611 commented Oct 6, 2020

markov00 left a comment

Choose a reason for hiding this comment

cla-checker-service bot commented Oct 12, 2020 • edited Loading

wylieconlon left a comment

Choose a reason for hiding this comment

wylieconlon Oct 12, 2020

Choose a reason for hiding this comment

wylieconlon Oct 12, 2020

Choose a reason for hiding this comment

dej611 Oct 13, 2020 • edited Loading

Choose a reason for hiding this comment

codecov-io commented Oct 13, 2020 • edited Loading

Codecov Report

markov00 commented Oct 19, 2020

markov00 left a comment •

edited

Loading

codecov-commenter commented Sep 28, 2020 •

edited

Loading

markov00 left a comment •

edited

Loading

nickofthyme left a comment •

edited

Loading

cla-checker-service bot commented Oct 12, 2020 •

edited

Loading

dej611 Oct 13, 2020 •

edited

Loading

codecov-io commented Oct 13, 2020 •

edited

Loading