feat(bar_chart): add Alignment offset to value labels #784

dej611 · 2020-08-19T09:53:18Z

Make it possible to position value labels based on bar relative geometry

Summary

This is a draft proposal for value label positioning within the bar chart with relative alignment.

There are still some open questions, such:

what should the default value be? Each chart rotation configuration has its own default alignment, so, for the moment, I've just extended the TextAlignment in this scenario to handle undefined values
Should explicit positioning have a different behaviour from similar "default" positioning?
- for instance when there are short bars the default configuration may propose an "overlapping" solution, but what about explicit alignment?

Checklist

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

Make it possible to position value labels based on bar relative geometry

markov00

I've put a small comment on how to refactor the long list of switch/if
I'm ok with the approach and the proposed API for that

src/chart_types/xy_chart/renderer/canvas/values/bar.ts

Compute offset distinctly per direction

Sync new labels alignment feature API with the doc

markov00

Everything seems to work fine except for a 180 degree rotation where: the vertical alignment is not working well

Also please note that the debug rectangle doesn't reflect the alignment: try to flag the debug check on the label story and you will see the issue when changing the alignment.

One more thing to add:
it will be great to have VRTs for every situation: all rotation + all configuration of alignment.
Please take a look at integration/tests/interactions.test.ts and a new set of tests for the various situations (you can run the VRTs for a specific file and/or a subset of tests with yarn test:integration:local your_test_file_name.test.ts --testNamePattern "your test name"

markov00 · 2020-09-29T07:52:49Z

src/chart_types/xy_chart/renderer/canvas/values/bar.ts

+const CHART_ROTATION: Record<string, Rotation> = {
+  BottomUp: 0,
+  TopToBottom: 180,
+  LeftToRight: 90,
+  RightToLeft: -90,
+};


I'm not 100% sure what are you referring to with these directions. All these terms resemble a mirror/flip action more than a rotation.
If you are using these to simplify the understanding of the rotation, please add a comment on the code where you are using them (like in each case of the switch few lines below

Yeah I don't think this is necessary, we could just make a comment on each of the switch cases below. Unless you are planning to expose this constant to the consumer for simplicity.

Yes, the original idea was to export them as they may be more readable than numbers.
Not really in the scope of this PR, but they helped to code the solution giving some more semantic.

If that's the case I think we should rename the enum from CHART_ROTATION to something more descriptive.

nickofthyme

Tested locally and works great, just left a few comments.

markov00

LGTM, adding some functional test will be great to avoid regressions:
please take a look at: integration/tests/interactions.test.ts on how to create VRTs with different knob options

codecov-io · 2020-10-12T17:11:27Z

Codecov Report

Merging #784 into master will decrease coverage by 2.39%.
The diff coverage is 3.65%.

@@            Coverage Diff             @@
##           master     #784      +/-   ##
==========================================
- Coverage   74.29%   71.90%   -2.40%     
==========================================
  Files         270      337      +67     
  Lines        9274    11929    +2655     
  Branches     1991     2589     +598     
==========================================
+ Hits         6890     8577    +1687     
- Misses       2377     3327     +950     
- Partials        7       25      +18

Flag	Coverage Δ
#unittests	`69.64% <3.65%> (-4.65%)`	⬇️

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

Impacted Files	Coverage Δ
src/utils/themes/theme.ts	`100.00% <ø> (ø)`
...chart_types/xy_chart/renderer/canvas/values/bar.ts	`8.63% <3.65%> (-2.48%)`	⬇️
src/utils/events.ts	`83.33% <0.00%> (-16.67%)`	⬇️
src/components/tooltip/tooltip.tsx	`63.84% <0.00%> (-6.95%)`	⬇️
.../chart_types/goal_chart/state/selectors/tooltip.ts	`33.33% <0.00%> (-5.13%)`	⬇️
src/chart_types/xy_chart/utils/series.ts	`91.54% <0.00%> (-4.00%)`	⬇️
.../chart_types/partition_chart/state/chart_state.tsx	`71.42% <0.00%> (-3.58%)`	⬇️
...t_types/partition_chart/state/selectors/tooltip.ts	`28.00% <0.00%> (-2.44%)`	⬇️
src/chart_types/xy_chart/utils/fill_series.ts	`97.82% <0.00%> (-2.18%)`	⬇️
src/state/chart_state.ts	`90.90% <0.00%> (-1.60%)`	⬇️
... and 102 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 e7a8a30...c2ed230. Read the comment docs.

wylieconlon

Works well!

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

🎉 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 alignment offset to value labels

f39069f

Make it possible to position value labels based on bar relative geometry

This was referenced Aug 19, 2020

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

Closed

Have a displayValueSettings option to dynamically set the label's offset based on the label box #792

Open

markov00 reviewed Aug 24, 2020

View reviewed changes

src/chart_types/xy_chart/renderer/canvas/values/bar.ts Show resolved Hide resolved

src/chart_types/xy_chart/renderer/canvas/values/bar.ts Outdated Show resolved Hide resolved

refactor(bar-labels): split function into vertical and horizontal fns

d511385

Compute offset distinctly per direction

dej611 self-assigned this Sep 18, 2020

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

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

docs: sync API page with new features

9052edb

Sync new labels alignment feature API with the doc

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

markov00 requested changes Sep 29, 2020

View reviewed changes

nickofthyme reviewed Sep 29, 2020

View reviewed changes

dej611 added 2 commits October 6, 2020 19:15

fix: refactor logic to reason about chart direction

20eddac

fix: fix positioning for top to bottom direction

ed40613

dej611 requested review from markov00 and nickofthyme October 7, 2020 13:50

markov00 approved these changes Oct 12, 2020

View reviewed changes

test: add new value labels visual snapshots

c2ed230

wylieconlon reviewed Oct 12, 2020

View reviewed changes

dej611 merged commit 363aeb4 into elastic:master Oct 13, 2020

dej611 deleted the feature/value-label-alignment branch October 13, 2020 13:11

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): add Alignment offset to value labels #784

feat(bar_chart): add Alignment offset to value labels #784

dej611 commented Aug 19, 2020

markov00 left a comment

markov00 left a comment •

edited

Loading

markov00 Sep 29, 2020

nickofthyme Sep 29, 2020

dej611 Sep 29, 2020

nickofthyme Sep 29, 2020

nickofthyme left a comment

markov00 left a comment

codecov-io commented Oct 12, 2020 •

edited

Loading

wylieconlon left a comment

markov00 commented Oct 19, 2020

feat(bar_chart): add Alignment offset to value labels #784

feat(bar_chart): add Alignment offset to value labels #784

Conversation

dej611 commented Aug 19, 2020

Summary

Checklist

markov00 left a comment

Choose a reason for hiding this comment

markov00 left a comment • edited Loading

Choose a reason for hiding this comment

markov00 Sep 29, 2020

Choose a reason for hiding this comment

nickofthyme Sep 29, 2020

Choose a reason for hiding this comment

dej611 Sep 29, 2020

Choose a reason for hiding this comment

nickofthyme Sep 29, 2020

Choose a reason for hiding this comment

nickofthyme left a comment

Choose a reason for hiding this comment

markov00 left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 12, 2020 • edited Loading

Codecov Report

wylieconlon left a comment

Choose a reason for hiding this comment

markov00 commented Oct 19, 2020

markov00 left a comment •

edited

Loading

codecov-io commented Oct 12, 2020 •

edited

Loading