Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: small multiples for XY charts (alpha) #793

Merged
merged 37 commits into from
Nov 6, 2020

Conversation

markov00
Copy link
Member

@markov00 markov00 commented Aug 26, 2020

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.

Screenshot 2020-11-03 at 16 08 50

Screenshot 2020-11-03 at 16 08 59

Screenshot 2020-11-03 at 16 09 04

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

@markov00 markov00 marked this pull request as draft August 26, 2020 09:25
@markov00 markov00 changed the title refactor: extract axes required space feat: small multiples Aug 26, 2020
@markov00 markov00 force-pushed the 2020_08_17-small-multiples branch from 9aa7c0d to bbf23fc Compare October 12, 2020 11:02
@codecov-io
Copy link

codecov-io commented Oct 15, 2020

Codecov Report

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

Impacted file tree graph

@@            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.

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

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Was this line removed intentionally?
image

Was this border only in debug mode?
image

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

How do we want to handle the end and start tick label overlap?
Image 2020-11-04 at 10 26 17 AM

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 Outdated Show resolved Hide resolved
@markov00
Copy link
Member Author

markov00 commented Nov 4, 2020

Was this line removed intentionally?
image

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?
image

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.
image

Right, I will check that

How do we want to handle the end and start tick label overlap?
Image 2020-11-04 at 10 26 17 AM
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 😅

🙏 🙏 🙏 🙏 🙏 🙏

Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
src/chart_types/xy_chart/annotations/rect/tooltip.ts Outdated Show resolved Hide resolved
src/chart_types/xy_chart/utils/axis_utils.ts Show resolved Hide resolved
src/chart_types/xy_chart/utils/series.ts Outdated Show resolved Hide resolved
src/chart_types/xy_chart/utils/series.ts Show resolved Hide resolved
src/utils/commons.ts Show resolved Hide resolved
src/utils/commons.ts Show resolved Hide resolved
src/utils/geometry.ts Outdated Show resolved Hide resolved
@nickofthyme
Copy link
Collaborator

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
Copy link
Collaborator

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.

Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clipping changes look good to me. 👍

@markov00 markov00 changed the title feat: small multiples feat: small multiples for XY charts (alpha) Nov 6, 2020
@markov00 markov00 merged commit d288208 into elastic:master Nov 6, 2020
@markov00 markov00 deleted the 2020_08_17-small-multiples branch November 6, 2020 18:03
Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, fantastic work! 🎉

* @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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should comments relating to MobX be removed?

};
}

return null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

markov00 pushed a commit that referenced this pull request Nov 24, 2020
# [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
Copy link
Member Author

🎉 This PR is included in version 24.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Nov 24, 2020
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
# [24.1.0](elastic/elastic-charts@v24.0.0...v24.1.0) (2020-11-24)

### Bug Fixes

* **area_charts:** correctly represent baseline with negative data points ([opensearch-project#896](elastic/elastic-charts#896)) ([b622fda](elastic/elastic-charts@b622fda))
* **legend:** legend sizes with ordinal data ([opensearch-project#867](elastic/elastic-charts#867)) ([74bcbad](elastic/elastic-charts@74bcbad)), closes [opensearch-project#811](elastic/elastic-charts#811)
* render orphan data points on lines and areas ([opensearch-project#900](elastic/elastic-charts#900)) ([3e2c739](elastic/elastic-charts@3e2c739)), closes [opensearch-project#783](elastic/elastic-charts#783)
* specs swaps correctly reflected in state ([opensearch-project#901](elastic/elastic-charts#901)) ([a94347f](elastic/elastic-charts@a94347f))

### Features

* **legend:** allow legend text to be copyable ([opensearch-project#877](elastic/elastic-charts#877)) ([21a96d3](elastic/elastic-charts@21a96d3)), closes [opensearch-project#710](elastic/elastic-charts#710)
* allow clearing series colors from memory ([opensearch-project#899](elastic/elastic-charts#899)) ([e97f4ab](elastic/elastic-charts@e97f4ab))
* merge series domain with the domain of another group ([opensearch-project#912](elastic/elastic-charts#912)) ([716ad5a](elastic/elastic-charts@716ad5a))
* small multiples for XY charts (alpha) ([opensearch-project#793](elastic/elastic-charts#793)) ([3b88e1c](elastic/elastic-charts@3b88e1c)), closes [opensearch-project#500](elastic/elastic-charts#500) [opensearch-project#500](elastic/elastic-charts#500)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants