fix(legend): legend sizes with ordinal data #867

rshen91 · 2020-10-13T22:15:19Z

Summary

Fixes #811
Before this PR, if the legend has LegendExtra and no spacingBuffer specified in the theme, the labels in the legend will be truncated when hovering over the series.

After the fix, the legend is sized to correctly show the labels and values for the legend:

Checklist

Delete any items that are not applicable to this PR.

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

codecov-io · 2020-10-13T22:20:37Z

Codecov Report

Merging #867 into master will increase coverage by 0.19%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #867      +/-   ##
==========================================
+ Coverage   69.56%   69.76%   +0.19%     
==========================================
  Files         321      336      +15     
  Lines       10630    10269     -361     
  Branches     2195     1964     -231     
==========================================
- Hits         7395     7164     -231     
+ Misses       3226     3038     -188     
- 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 Δ
src/chart_types/xy_chart/state/utils/utils.ts	`92.27% <100.00%> (-0.01%)`	⬇️
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%)`	⬇️
src/components/tooltip/tooltip.tsx	`59.37% <0.00%> (-4.63%)`	⬇️
src/components/chart_status.tsx	`87.50% <0.00%> (-4.17%)`	⬇️
... and 181 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 3bdd1ee...77a11ba. Read the comment docs.

codecov-commenter · 2020-10-15T13:55:06Z

Codecov Report

Merging #867 into master will increase coverage by 0.19%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #867      +/-   ##
==========================================
+ Coverage   69.56%   69.76%   +0.19%     
==========================================
  Files         321      336      +15     
  Lines       10630    10269     -361     
  Branches     2195     1964     -231     
==========================================
- Hits         7395     7164     -231     
+ Misses       3226     3038     -188     
- 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 Δ
src/chart_types/xy_chart/state/utils/utils.ts	`92.27% <100.00%> (-0.01%)`	⬇️
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%)`	⬇️
src/components/tooltip/tooltip.tsx	`59.37% <0.00%> (-4.63%)`	⬇️
src/components/chart_status.tsx	`87.50% <0.00%> (-4.17%)`	⬇️
... and 181 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 3bdd1ee...77a11ba. Read the comment docs.

nickofthyme · 2020-10-16T15:26:47Z

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

+      if (xDomain.scaleType === ScaleType.Ordinal) {
+        lastValues.set(seriesKey, { y0: initialY0, y1: initialY1 });
+      }


I'm not sure this is needed. Wouldn't it just set the correct values without this?

Agreed - removed in a054fdc

nickofthyme · 2020-10-16T15:27:04Z

src/chart_types/xy_chart/legend/legend.ts

+  lastValue: LastValues,
+  formatter: (value: any, options?: TickFormatterOptions | undefined) => string,
+) {
+  return showLegendExtra || xScaleType === 'ordinal'


You can't check the type here because the legendSizeSelector needs to know these values, to fix the issue. You could check the xScaleType before rendering in the LegendItem component.

Another way to fix this would be to add a sizingLabel prop to this and use that value in the legendSizeSelector instead of formatted.

Commit a054fdc and discussed in zoom 🙇‍♀️

nickofthyme · 2020-10-16T15:28:36Z

src/chart_types/xy_chart/legend/legend.ts

-        defaultExtra: {
-          raw: lastValue && lastValue.y0 !== null ? lastValue.y0 : null,
-          formatted: lastValue && lastValue.y0 !== null ? formatter(lastValue.y0) : null,
-        },


Careful with extracting this, the value here is y0 not y1 like the function returns

Thanks! Commit a054fdc for changes

codecov-io · 2020-10-16T19:26:24Z

Codecov Report

Merging #867 into master will increase coverage by 0.53%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #867      +/-   ##
==========================================
+ Coverage   69.55%   70.09%   +0.53%     
==========================================
  Files         321      336      +15     
  Lines       10632    10920     +288     
  Branches     2196     2238      +42     
==========================================
+ Hits         7395     7654     +259     
- Misses       3228     3251      +23     
- Partials        9       15       +6

Flag	Coverage Δ
#unittests	`69.58% <100.00%> (+0.03%)`	⬆️

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

Impacted Files	Coverage Δ
src/chart_types/xy_chart/state/utils/utils.ts	`92.18% <ø> (-0.10%)`	⬇️
src/chart_types/xy_chart/legend/legend.ts	`87.50% <100.00%> (+6.54%)`	⬆️
...t_types/xy_chart/state/selectors/compute_legend.ts	`100.00% <100.00%> (ø)`
...y_chart/state/selectors/get_legend_items_labels.ts	`100.00% <100.00%> (ø)`
src/mocks/store/store.ts	`92.59% <0.00%> (ø)`
src/mocks/specs/index.ts	`100.00% <0.00%> (ø)`
src/mocks/scale/index.ts	`100.00% <0.00%> (ø)`
src/mocks/series/index.ts	`100.00% <0.00%> (ø)`
src/mocks/index.ts	`100.00% <0.00%> (ø)`
src/mocks/store/index.ts	`100.00% <0.00%> (ø)`
... and 9 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 eda6889...6b57387. Read the comment docs.

nickofthyme

Awesome! LGTM, I made one comment on the tests to fix but other than that I approve.

nickofthyme · 2020-10-20T15:22:30Z

src/chart_types/xy_chart/legend/legend.test.ts

+  it('should return correct legendSizingLabel with linear scale and showExtraLegend set to true', () => {
+    const formatter = (d: string | number) => `${Number(d).toFixed(2)} dogs`;
+    const lastValues = { y0: null, y1: 14 };
+    const showExtraLegend = true;
+    const xScaleIsLinear = ScaleType.Linear;
+
+    expect(getLegendExtra(showExtraLegend, xScaleIsLinear, formatter, 'y1', lastValues)).toMatchObject({
+      legendSizingLabel: '14.00 dogs',
+    });
+  });
+  it('should return formatted to null with ordinal scale and showExtraLegend set to true', () => {
+    const formatter = (d: string | number) => `${Number(d).toFixed(2)} dogs`;
+    const lastValues = { y0: null, y1: 14 };
+
+    expect(getLegendExtra(true, ScaleType.Ordinal, formatter, 'y1', lastValues)).toMatchObject({
+      formatted: null,
+    });
+  });
+  it('should return legendSizingLabel null with showLegendExtra set to false', () => {
+    const formatter = (d: string | number) => `${Number(d).toFixed(2)} dogs`;
+    const lastValues = { y0: null, y1: 14 };
+    const showLegendExtra = false;
+
+    expect(getLegendExtra(showLegendExtra, ScaleType.Ordinal, formatter, 'y1', lastValues)).toMatchObject({
+      legendSizingLabel: null,
+    });
+  });


I think you should assert all three parts of the extraValue for all of these

Thank you! Addressed in commit 6b57387

# [24.1.0](v24.0.0...v24.1.0) (2020-11-24) ### Bug Fixes * **area_charts:** correctly represent baseline with negative data points ([#896](#896)) ([d1243f1](d1243f1)) * **legend:** legend sizes with ordinal data ([#867](#867)) ([7559e0d](7559e0d)), closes [#811](#811) * render orphan data points on lines and areas ([#900](#900)) ([0be282b](0be282b)), closes [#783](#783) * specs swaps correctly reflected in state ([#901](#901)) ([7fba882](7fba882)) ### Features * **legend:** allow legend text to be copyable ([#877](#877)) ([9cd3459](9cd3459)), closes [#710](#710) * allow clearing series colors from memory ([#899](#899)) ([ab1af38](ab1af38)) * merge series domain with the domain of another group ([#912](#912)) ([325b013](325b013)) * small multiples for XY charts (alpha) ([#793](#793)) ([d288208](d288208)), closes [#500](#500) [#500](#500)

markov00 · 2020-11-24T14:52:24Z

🎉 This PR is included in version 24.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

rshen91 changed the title ~~fix: remove early return for ordinal lastValues~~ fix(legend): legend sizes with ordinal data Oct 13, 2020

nickofthyme reviewed Oct 16, 2020

View reviewed changes

fix: fix sizing for showLegendExtra and ordinal xScale

a054fdc

rshen91 force-pushed the legend-sizing branch from dcb904a to a054fdc Compare October 16, 2020 19:20

rshen91 added the :legend Legend related issue label Oct 16, 2020

rshen91 added 3 commits October 16, 2020 14:48

Merge remote-tracking branch 'upstream/master' into legend-sizing

56676aa

fix: add internal flag to fix api extractor failures

24d6e44

Merge remote-tracking branch 'upstream/master' into legend-sizing

c9ca6a4

rshen91 marked this pull request as ready for review October 19, 2020 13:51

rshen91 requested a review from nickofthyme October 19, 2020 14:22

nickofthyme approved these changes Oct 20, 2020

View reviewed changes

test: refactoring tests and mixed chart fixed story

6b57387

rshen91 merged commit 7559e0d into elastic:master Oct 20, 2020

rshen91 deleted the legend-sizing branch October 20, 2020 17:33

rshen91 added the bug Something isn't working label Oct 20, 2020

markov00 added the released Issue released publicly label Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(legend): legend sizes with ordinal data #867

fix(legend): legend sizes with ordinal data #867

rshen91 commented Oct 13, 2020 •

edited

Loading

codecov-io commented Oct 13, 2020 •

edited

Loading

codecov-commenter commented Oct 15, 2020

nickofthyme Oct 16, 2020

rshen91 Oct 19, 2020 •

edited

Loading

nickofthyme Oct 16, 2020

nickofthyme Oct 16, 2020

rshen91 Oct 19, 2020 •

edited

Loading

nickofthyme Oct 16, 2020

rshen91 Oct 19, 2020 •

edited

Loading

codecov-io commented Oct 16, 2020 •

edited

Loading

nickofthyme left a comment

nickofthyme Oct 20, 2020

rshen91 Oct 20, 2020

markov00 commented Nov 24, 2020

fix(legend): legend sizes with ordinal data #867

fix(legend): legend sizes with ordinal data #867

Conversation

rshen91 commented Oct 13, 2020 • edited Loading

Summary

Checklist

codecov-io commented Oct 13, 2020 • edited Loading

Codecov Report

codecov-commenter commented Oct 15, 2020

Codecov Report

nickofthyme Oct 16, 2020

Choose a reason for hiding this comment

rshen91 Oct 19, 2020 • edited Loading

Choose a reason for hiding this comment

nickofthyme Oct 16, 2020

Choose a reason for hiding this comment

nickofthyme Oct 16, 2020

Choose a reason for hiding this comment

rshen91 Oct 19, 2020 • edited Loading

Choose a reason for hiding this comment

nickofthyme Oct 16, 2020

Choose a reason for hiding this comment

rshen91 Oct 19, 2020 • edited Loading

Choose a reason for hiding this comment

codecov-io commented Oct 16, 2020 • edited Loading

Codecov Report

nickofthyme left a comment

Choose a reason for hiding this comment

nickofthyme Oct 20, 2020

Choose a reason for hiding this comment

rshen91 Oct 20, 2020

Choose a reason for hiding this comment

markov00 commented Nov 24, 2020

rshen91 commented Oct 13, 2020 •

edited

Loading

codecov-io commented Oct 13, 2020 •

edited

Loading

rshen91 Oct 19, 2020 •

edited

Loading

rshen91 Oct 19, 2020 •

edited

Loading

rshen91 Oct 19, 2020 •

edited

Loading

codecov-io commented Oct 16, 2020 •

edited

Loading