feat: small multiples for XY charts (alpha) by markov00 · Pull Request #793 · elastic/elastic-charts

markov00 · 2020-08-26T09:25:35Z

Summary

The PR adds a first alpha version of the SmallMultiple chart configuration as requested in #500

Two main components are used to describe the small multiples:

<GroupBy /> that is a generic component that allows the specification of a group by operation and sorting order. The by prop request a function with two arguments, the spec and the datum. It's called for each spec and for each data point available in the data array of a spec. It should return a unique value used to group the data points into multiple series.
<SmallMultiples /> component with two main optional props: splitVertically or splitHorizontally where you can specify the id of a GroupBy operator to render vertically (one below the other) or horizontally (one aside the other) the series defined by that group operation. A preliminary style configuration can be used to configure inner and outer padding in percentage for the vertical or horizontal charts.

3 examples are added to Storybook.

This PR changes the way most of the geometries are computed targeting what I've called panel (the rectangular rendering area that cover a single small multiple)instead of the previously used chartDimension.

Another important change was applied to the data pipeline: the Formatted and Raw DataSeries types are now removed in favor of a single DataSeries type that contains all the required props to identify a Series and its characteristics.

Note:

the major change is the tooltip series sorting that looks different than the existing one. Further investigation can be done to understand what is the best way to sort/handle that.

Further developments, on subsequent PRs:

tooltip name should reflect the small multiple categories (discussion point)
include the axis line and/or the full axis rendering below and aside each small multiple (actually it only globally top/left/right/bottom axes) [small multiples] Render axis on every panel #891
refactor the axes geometry generator to correctly compute the anchor point of each axis (see also src/chart_types/xy_chart/state/selectors/compute_per_panel_axes_geoms.ts that currently render axis only on the first column and the last row) [small multiples] Render axis on every panel #891
more VRTs to cover all the rendering features (line/bar/areas/bubble/fitted areas/lines/annotations/axis/legend)
add series sorting Sort DataSeries #795

Checklist

Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
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-15T09:25:33Z

Codecov Report

Merging #793 into master will increase coverage by 0.24%.
The diff coverage is 71.24%.

@@            Coverage Diff             @@
##           master     #793      +/-   ##
==========================================
+ Coverage   69.58%   69.82%   +0.24%     
==========================================
  Files         321      351      +30     
  Lines       10636    10542      -94     
  Branches     2196     2031     -165     
==========================================
- Hits         7401     7361      -40     
+ Misses       3226     3099     -127     
- Partials        9       82      +73

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/domains/x_domain.ts	`100.00% <ø> (ø)`
.../chart_types/xy_chart/renderer/canvas/axes/line.ts	`13.63% <0.00%> (ø)`
.../chart_types/xy_chart/renderer/canvas/axes/tick.ts	`26.31% <0.00%> (+2.50%)`	⬆️
..._types/xy_chart/renderer/canvas/axes/tick_label.ts	`21.73% <0.00%> (ø)`
...chart_types/xy_chart/renderer/canvas/axes/title.ts	`13.46% <0.00%> (ø)`
..._types/xy_chart/renderer/canvas/primitives/line.ts	`25.00% <0.00%> (+8.87%)`	⬆️
..._types/xy_chart/renderer/canvas/primitives/path.ts	`9.80% <0.00%> (+0.36%)`	⬆️
.../xy_chart/renderer/dom/annotations/annotations.tsx	`67.92% <0.00%> (+0.65%)`	⬆️
.../selectors/get_tooltip_values_highlighted_geoms.ts	`89.04% <ø> (-1.75%)`	⬇️
src/chart_types/xy_chart/utils/axis_utils.ts	`91.84% <ø> (-0.60%)`	⬇️
... and 266 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 3734537...95aba23. Read the comment docs.

nickofthyme

Just looking at the visual changes, made a few observations.

Was this line removed intentionally?

Was this border only in debug mode?

Looks like the clipped area is slightly smaller here. This looks to be the cause of many of the minor pixel changes.

How do we want to handle the end and start tick label overlap?

Are we planning to fix the tooltip ordering before 7.11?

These are duplicates and should be deleted:

integration/tests/__image_snapshots__/bar-stories-test-ts-bar-series-stories-test-histogram-mode-ordinal-enable-histogram-mode-is-true-has-histogram-bar-series-is-true-rotation-negative-90-2-snap.png
integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-default-rotation-0-placement-right-shows-tooltip-in-top-right-corner-2-snap.png
integration/tests/__image_snapshots__/mixed-stories-test-ts-mixed-series-stories-fitting-functions-stacked-charts-area-charts-end-value-set-to-nearest-should-display-correct-fit-for-type-linear-2-snap.png

...now off to the 9k code changes 😅

api/charts.api.md

markov00 · 2020-11-04T18:08:26Z

Was this line removed intentionally?

No that was an issue with the preexisting code where a LineSeries with a different groupId and an axis with the same groupId wasn't visualized: check the story code here: https://elastic.github.io/elastic-charts/?path=/story/axes--custom-domain to me, the right behavior is to show that line.

Was this border only in debug mode?

This only appears in debug mode (it will helps when debugging multiple panels.

Looks like the clipped area is slightly smaller here. This looks to be the cause of many of the minor pixel changes.

Right, I will check that

How do we want to handle the end and start tick label overlap?

Are we planning to fix the tooltip ordering before 7.11?

Yep, I think we should fix that before 7.11. What we can do is to: compute the exact overflow of the last displayed label on the chart, compare it to the current padding percentage, and adjust it accordingly. In case we can also hide the last tick on every panel except the latest one

These are duplicates and should be deleted:

integration/tests/__image_snapshots__/bar-stories-test-ts-bar-series-stories-test-histogram-mode-ordinal-enable-histogram-mode-is-true-has-histogram-bar-series-is-true-rotation-negative-90-2-snap.png

integration/tests/__image_snapshots__/interactions-test-ts-interactions-tooltips-positioning-boundary-el-default-rotation-0-placement-right-shows-tooltip-in-top-right-corner-2-snap.png

integration/tests/__image_snapshots__/mixed-stories-test-ts-mixed-series-stories-fitting-functions-stacked-charts-area-charts-end-value-set-to-nearest-should-display-correct-fit-for-type-linear-2-snap.png

Will do!

...now off to the 9k code changes 😅

🙏 🙏 🙏 🙏 🙏 🙏

nickofthyme

Overall, really great enhancement and code refactoring. Love the changes to the test files and all the mock improvements/additions. Also love the separation into panels, makes the logic much cleaner.

Reviewed all the src code and everything LGTM. I left a few comments and questions.

Gonna now test storybook locally for any regressions.

src/chart_types/xy_chart/utils/panel.ts

src/chart_types/xy_chart/annotations/rect/dimensions.ts

src/chart_types/xy_chart/annotations/rect/tooltip.ts

src/chart_types/xy_chart/annotations/tooltip.ts

src/chart_types/xy_chart/utils/axis_utils.ts

src/chart_types/xy_chart/utils/series.ts

src/utils/commons.ts

src/utils/geometry.ts

nickofthyme · 2020-11-05T15:12:03Z

Thanks for the response on the VRT changes. I think the clipped area changes and the end/start tick labels are the only thing remaining.

nickofthyme · 2020-11-05T15:53:45Z

I've checked the storybook going though about half of all stories and everything looks good outside of the two issues mentioned above. I was not able to check the legend interaction with the three split chart stories so might be worth adding a legend to one of them.

The clipping issue seems to be different for areas/lines than points, as points are rendered beyond the clipped area/line geometry.

nickofthyme

Clipping changes look good to me. 👍

monfera

LGTM, fantastic work! 🎉

monfera · 2020-11-09T08:36:42Z

src/chart_types/xy_chart/domains/y_domain.ts

+ * @remarks
+ * After mobx->redux https://github.com/elastic/elastic-charts/pull/281 we keep the specs untouched on mount
+ * in MobX version, the stackAccessors was programmatically added to every histogram specs
+ * in ReduX version, we left untouched the specs, so we have to manually check that


Should comments relating to MobX be removed?

src/chart_types/xy_chart/renderer/canvas/bars.ts

src/chart_types/xy_chart/state/selectors/compute_panels.ts

monfera · 2020-11-09T08:43:53Z

src/chart_types/xy_chart/state/selectors/compute_per_panel_axes_geoms.ts

+        };
+      }
+
+      return null;


Wouldn't mind a slight reshaping, like return firstColumn || lastLine ? .... : null or if / else or if(!firstColumn && !lastLine) { return null}

monfera · 2020-11-09T08:44:53Z

src/chart_types/xy_chart/state/selectors/compute_per_panel_axes_geoms.ts

+    return getPerPanelMap(scales, (anchor, h, v) => {
+      const lastLine = horizontal.domain.includes(h) && vertical.domain[vertical.domain.length - 1] === v;
+      const firstColumn = horizontal.domain[0] === h;
+      if (firstColumn || lastLine) {


I'd like to understand a little bit more about the logic here, maybe on a quick call?

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

🎉 This PR is included in version 24.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

markov00 marked this pull request as draft August 26, 2020 09:25

markov00 changed the title ~~refactor: extract axes required space~~ feat: small multiples Aug 26, 2020

feat: small-multiples for xy axis chart

bbf23fc

markov00 force-pushed the 2020_08_17-small-multiples branch from 9aa7c0d to bbf23fc Compare October 12, 2020 11:02

refactor: test adapted to new set of types

d8c22dc

markov00 added 19 commits October 16, 2020 15:09

test: refactor commented out tests

c68a3cc

Merge branch 'master' into 2020_08_17-small-multiples

3cd5738

fix: translate point rendering to correct panel

024cb2c

fix: grid lines

7c79d79

fix: grid lines

849be06

WIP

d9ecdf3

fix: tooltip positioning on panels

f43ca65

fix: rotation

6324237

fix: line annotations

00a0a42

fix: rect annotations

f7b961d

Merge branch 'master' into 2020_08_17-small-multiples

1775933

test: align mocks to new types

c33c90e

build: fix linting

2cc55a1

docs: update API report

54db5f3

fix: bar value per panel

51161b3

fix: annotation marker pos

524a5e8

fix: simplified types

7f42203

docs: add storybook examples

d9797ff

fix: missing clip on rendering geoms

923339d

nickofthyme mentioned this pull request Nov 3, 2020

Small multiples in vis_type_xy plugin elastic/kibana#82496

Closed

2 tasks

markov00 added 4 commits November 3, 2020 21:08

test: update VRTs

53f26b1

docs: revert playground changes

480dd1b

fix: clean types and remove flatMap

3cbf086

refactor: cleanup code

fd5fff9

refactor: clean up code

90ab7ef

markov00 marked this pull request as ready for review November 3, 2020 21:08

markov00 requested review from monfera, nickofthyme and rshen91 November 3, 2020 21:09

nickofthyme reviewed Nov 4, 2020

View reviewed changes

api/charts.api.md Outdated Show resolved Hide resolved

nickofthyme reviewed Nov 4, 2020

View reviewed changes

markov00 added 4 commits November 5, 2020 11:16

test: removed duplicated VRTs

08bb9cc

docs: fix API report

c030397

refactor: addressing review feedbacks

5bfda72

fix: logic on merge domain with no dataseries

1b90cfe

fix: wrong fix on merge domain

cd935e7

markov00 added 3 commits November 6, 2020 14:48

fix: disable clipping to line rendering to match master

5dade3a

docs: restore master VRTs

d356ace

test: fix vrts after removing clippings on lines

7dd4f48

nickofthyme approved these changes Nov 6, 2020

View reviewed changes

nickofthyme and others added 3 commits November 6, 2020 09:49

Merge branch 'master' into 2020_08_17-small-multiples

e91488d

docs: add legend knob on small multiple stories

80a7649

test: adding vrts for small multiples

95aba23

markov00 changed the title ~~feat: small multiples~~ feat: small multiples for XY charts (alpha) Nov 6, 2020

markov00 merged commit d288208 into elastic:master Nov 6, 2020

markov00 deleted the 2020_08_17-small-multiples branch November 6, 2020 18:03

monfera reviewed Nov 9, 2020

View reviewed changes

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

Conversation

markov00 commented Aug 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

codecov-io commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nickofthyme left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

markov00 commented Nov 4, 2020

Uh oh!

nickofthyme left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nickofthyme commented Nov 5, 2020

Uh oh!

nickofthyme commented Nov 5, 2020

Uh oh!

nickofthyme left a comment

Choose a reason for hiding this comment

Uh oh!

monfera left a comment

Choose a reason for hiding this comment

Uh oh!

monfera Nov 9, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

monfera Nov 9, 2020

Choose a reason for hiding this comment

Uh oh!

monfera Nov 9, 2020

Choose a reason for hiding this comment

Uh oh!

markov00 commented Nov 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

markov00 commented Aug 26, 2020 •

edited

Loading

codecov-io commented Oct 15, 2020 •

edited

Loading