feat(xy): apply the data value formatter to data values over bars #1419

rshen91 · 2021-10-04T18:30:25Z

Summary

The tickFormat on the data values over bars now will correctly display when charts are rotated.

Details

PR #1390 includes some old commits changing the nature of getAxesBySpecId() but this function is used in multiple places where rotations were already accounted for. This new approach in this PR does not impact the tooltip header or body formatting, rectangular annotations, legend formatting etc.

Issues

Closes #1387

Checklist

The proper chart type label has been added (e.g. :xy, :partition)
All related issues have been linked (i.e. closes #123, fixes #123)
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

rshen91 · 2021-10-04T20:19:49Z

@markov00 just wanted to confirm with the with value label story that the rotation in commit 06bef45 seems fine. There isn't a tick formatter for the bottom axis and the data for the non-stacked series isn't whole number data, so I think the formatting is working ok.

[
   {x: 0, y: 3.934269, g: 'b'},
   {x: 1, y: 7.662253926666667, g: 'b'},
   {x: 2, y: 7.043862173333342, g: 'b'},
   {x: 3, y: 6.394734540000064, g: 'b'},
   {x: 4, y: 6.059608170666939, g: 'b'},
   {x: 5, y: 0.25, g: 0},
   {x: 6, y: 8, g: 0}
]

Let me know what you think, thank you.

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

markov00

To nicely verify that the correct formatter is applied on rotation please add two different formatters that can help identify the formatter on the value something like:

for the horizontal axis: ${number.toFixed(2)} H
for the vertical axis: ${number.toFixed(2) V}

This renders better nicely the values and shows the H or the V on the bar values that is easier to match that a missed formatter.

packages/charts/src/chart_types/xy_chart/state/utils/spec.ts

rshen91 · 2021-10-11T17:50:07Z

Checked through the failures in the vrts commit 8e768d2 - small formatting changes applied on the https://elastic.github.io/elastic-charts/?path=/story/bar-chart--with-value-label&globals=theme:light stories with rotations. Updated vrts in bar stories in 3c3a349

...ages/charts/src/chart_types/xy_chart/state/selectors/get_tooltip_values_highlighted_geoms.ts

packages/charts/src/chart_types/xy_chart/state/utils/spec.ts

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

nickofthyme

Code changes LGTM.

nickofthyme · 2021-10-14T20:09:57Z

integration/tests/bar_stories.test.ts

+    eachRotation.describe((rotation) => {
+      describe.each<NonNullable<DisplayValueStyle['alignment']>['vertical']>([
+        VerticalAlignment.Middle,
+        VerticalAlignment.Top,
+        VerticalAlignment.Bottom,
+      ])('Vertical Alignment - %s', (verticalAlignment) => {
+        describe.each<NonNullable<DisplayValueStyle['alignment']>['horizontal']>([
+          HorizontalAlignment.Left,
+          HorizontalAlignment.Center,
+          HorizontalAlignment.Right,


nickofthyme · 2021-10-14T20:12:39Z

storybook/stories/bar/58_data_values.story.tsx

+        fontSize: 10,
+        fill: 'black',


I would suggest making this larger because it blends in. For others that view this story I'd be easier to view the change when the value is more pronounced.

markov00

Looks great, I've added a comment that should probably addressed in this PR to have a nice and clean set of VRTs

markov00 · 2021-10-15T07:40:59Z

integration/tests/bar_stories.test.ts

@@ -129,6 +129,27 @@ describe('Bar series stories', () => {
    });
  });

+  describe('value label positioning and formatting', () => {


This is great, works well and visually is easy to read and check for defects!
I think we can really remove the screenshots and tests that looks similar to this but with a different story (the describe test before this one: value labels positioning.
I think we can remove it, but keeping the additional test in the describe called clip both geometry and chart area values. The reason is the following: that set screenshot is difficult to read, the high decimal values + very thin bars makes it difficult to understand what is the test about (rendering value labels with different configurations)

Good call - updated vrts in 14182f by deleting the old bar_stories vrts and rerunning.

# [38.0.0](v37.0.0...v38.0.0) (2021-10-15) ### Bug Fixes * **deps:** update dependency @elastic/eui to v39 ([#1422](#1422)) ([2ee97aa](2ee97aa)) * **goal:** reduce whitespace for circular charts ([#1413](#1413)) ([6517523](6517523)) * **interactions:** change allowBrushingLastHistogramBin to true ([#1396](#1396)) ([9fa9783](9fa9783)) * **xy:** remove wrongly represented null/missing values in tooltip ([#1415](#1415)) ([e5963a3](e5963a3)), closes [#1414](#1414) ### Code Refactoring * scales ([#1410](#1410)) ([a53a2ba](a53a2ba)) ### Features * **scales:** add `LinearBinary` scale type ([#1389](#1389)) ([9f2e427](9f2e427)) * **xy:** adaptive tick raster ([#1420](#1420)) ([200577b](200577b)) * **xy:** apply the data value formatter to data values over bars ([#1419](#1419)) ([e673fc7](e673fc7)) ### BREAKING CHANGES * **interactions:** allowBrushingLastHistogramBucket renamed to allowBrushingLastHistogramBin on the Settings component defaults true and is only applied for histogram type charts * LogScaleOptions.logBase` is now a `number` instead of the object enum `LogBase`. Some edge case data or configuration _might_, with a deemed low likelihood, lead to a situation where the earlier version would have silently not rendered a bar, line or point, while the new code doesn't `catch`, therefore throw an exception (see the last item). General risk of regressions due to the quantity of code changes (altogether 3.5k)

nickofthyme · 2021-10-15T14:01:16Z

🎉 This PR is included in version 38.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

feat: apply the data value formatter

06bef45

rshen91 added :chart Chart element related issue :xy Bar/Line/Area chart related labels Oct 4, 2021

rshen91 requested review from nickofthyme and markov00 October 4, 2021 20:31

Merge remote-tracking branch 'upstream/master' into tickFormatRevised

a9eee2f

markov00 requested changes Oct 6, 2021

View reviewed changes

packages/charts/src/chart_types/xy_chart/state/utils/utils.ts Outdated Show resolved Hide resolved

rshen91 marked this pull request as draft October 6, 2021 13:26

rshen91 added 4 commits October 6, 2021 07:49

Merge remote-tracking branch 'upstream/master' into tickFormatRevised

a78455b

feat: chart rotations in getAxesSpecForSpecId

29eb5eb

Merge remote-tracking branch 'upstream/master' into tickFormatRevised

2c7bdf5

fix: fix conditional in getAxesSpecForSpecId

5287727

markov00 requested changes Oct 11, 2021

View reviewed changes

packages/charts/src/chart_types/xy_chart/state/utils/spec.ts Outdated Show resolved Hide resolved

rshen91 added 3 commits October 11, 2021 11:17

fix: update getAxesSpecForSpecId

9822ee5

test: fix fallback for isXDomain check

8e768d2

test: update vrt bar stories

3c3a349

rshen91 marked this pull request as ready for review October 11, 2021 18:43

rshen91 requested a review from markov00 October 11, 2021 18:43

markov00 reviewed Oct 12, 2021

View reviewed changes

rshen91 added 2 commits October 12, 2021 08:11

refactor: remove isXDomain from tooltip and utils

89a2fb5

refactor: clean up getAxesSpecForSpecId

e152fc4

rshen91 requested a review from markov00 October 12, 2021 19:38

rshen91 added 4 commits October 13, 2021 07:34

test: add V and H to formatters in data value story

dd4e132

fix: code review feedback

afd4d57

test: update vrt all test

dad8361

test: fixed rotations

9dc836a

nickofthyme approved these changes Oct 14, 2021

View reviewed changes

test: make data values more visible in story

d80f9e2

test: update vrt test

6f69be9

markov00 approved these changes Oct 15, 2021

View reviewed changes

rshen91 added 2 commits October 15, 2021 07:31

test: remove old bar vrts

1418a2f

Merge remote-tracking branch 'upstream/master' into tickFormatRevised

50a4d49

rshen91 merged commit e673fc7 into elastic:master Oct 15, 2021

nickofthyme added the released Issue released publicly label Oct 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(xy): apply the data value formatter to data values over bars #1419

feat(xy): apply the data value formatter to data values over bars #1419

rshen91 commented Oct 4, 2021 •

edited

Loading

rshen91 commented Oct 4, 2021 •

edited

Loading

markov00 left a comment

rshen91 commented Oct 11, 2021 •

edited

Loading

nickofthyme left a comment

nickofthyme Oct 14, 2021

nickofthyme Oct 14, 2021

markov00 left a comment

markov00 Oct 15, 2021

rshen91 Oct 15, 2021

nickofthyme commented Oct 15, 2021

feat(xy): apply the data value formatter to data values over bars #1419

feat(xy): apply the data value formatter to data values over bars #1419

Conversation

rshen91 commented Oct 4, 2021 • edited Loading

Summary

Details

Issues

Checklist

rshen91 commented Oct 4, 2021 • edited Loading

markov00 left a comment

Choose a reason for hiding this comment

rshen91 commented Oct 11, 2021 • edited Loading

nickofthyme left a comment

Choose a reason for hiding this comment

nickofthyme Oct 14, 2021

Choose a reason for hiding this comment

nickofthyme Oct 14, 2021

Choose a reason for hiding this comment

markov00 left a comment

Choose a reason for hiding this comment

markov00 Oct 15, 2021

Choose a reason for hiding this comment

rshen91 Oct 15, 2021

Choose a reason for hiding this comment

nickofthyme commented Oct 15, 2021

rshen91 commented Oct 4, 2021 •

edited

Loading

rshen91 commented Oct 4, 2021 •

edited

Loading

rshen91 commented Oct 11, 2021 •

edited

Loading