Small multiples in vis_type_xy plugin #86880

DianaDerevyankina · 2020-12-23T10:14:54Z

Closes #82496

Summary

Added split chart aggregation support in visualize for newChartUi

Removed existing warning message
(before)
Enabled Split chart option and removed the tooltip
(before)
Update advanced setting wording
(before)

(after)

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

alexwizp · 2021-01-06T10:54:27Z

src/plugins/vis_type_xy/public/config/get_aspects.ts

  const seriesDimensions = Array.isArray(series) || series === undefined ? series : [series];

  return {
    x: getAspectsFromDimension(columns, x) ?? getEmptyAspect(),
    y: getAspectsFromDimension(columns, y) ?? [],
    z: z && z?.length > 0 ? getAspectsFromDimension(columns, z[0]) : undefined,
    series: getAspectsFromDimension(columns, seriesDimensions),
+    splitColumn:
+      splitColumn && splitColumn?.length > 0


nit: splitColumn && splitColumn?.length > 0 === splitColumn?.length

alexwizp · 2021-01-06T10:55:10Z

src/plugins/vis_type_xy/public/config/get_aspects.ts

+        ? getAspectsFromDimension(columns, splitColumn[0])
+        : undefined,
+    splitRow:
+      splitRow && splitRow?.length > 0 ? getAspectsFromDimension(columns, splitRow[0]) : undefined,


nit: splitRow && splitRow?.length > 0 -> splitRow?.length

stratoula · 2021-01-07T09:24:52Z

@elasticmachine merge upstream

alexwizp · 2021-01-13T11:41:14Z

@nickofthyme please review that. Do you have any concerns around that?

.ci/packer_cache_for_branch.sh

stratoula · 2021-01-13T12:49:09Z

I haven't checked the code yet but I don't think that it works as we want. I can see my charts split on a Range agg but on a terms aggregation, for example, they don't. Or at least, it doesn't work on any case.

I have this configuration and on the legacy creates two charts

but on the new xy axis

Moreover, I don't think that the legend should be repeated

nickofthyme · 2021-01-14T00:41:11Z

@dziyanadzeraviankina,

I think I solved the issue with the intervals being different between the two implementations.

See this pr for details DianaDerevyankina#1.

I am looking to see how we could increase the scaled interval based on the number of vertical split chart panels.

I also noticed that if the interval is small enough where the scale value is used in the es query, the click filter is using the user-defined interval value.

Happy to discuss it tomorrow.

@stratoula,

I will look into the issue with the terms aggregation.

The other issue related to the multiple legend values is an issue that Marco is resolving on the elastic-charts side.

stratoula · 2021-01-14T06:57:33Z

Thanx @nickofthyme, @dziyanadzeraviankina one more thing. Now that the new palette service is integrated on the xy plugin, you should also check if it works correctly with the split chart (merged yesterday on master)

# Conflicts: # src/plugins/vis_type_xy/public/components/split_chart_warning.tsx # src/plugins/vis_type_xy/public/vis_types/split_tooltip.tsx

alexwizp

LGTM

sulemanof · 2021-01-20T12:22:31Z

Haven't checked the code yet, but found next issues (maybe some of that are made on purpose, but in that case I would appreciate for documenting them in the PR description).

axes don't have the same labels calculated in split chart mode:
- vislib split by columns
- xy chart split by columns
- vislib split by rows
- xy chart split by rows
Each metric is duplicated by each split chart:

which leads to "metrics legend hell" 🥶

In vislib each metric is repeated only once; hovering over a metric highlights all selected metrics across all charts - could we replicate the same in xy chart?

Except these small issues, everything seems to work fine 👍 ❤️

stratoula · 2021-01-20T12:51:36Z

@sulemanof about the multiple labels I had asked the same, check Nick's reply #86880 (comment), it seems that it should be fixed on es-charts side.

nickofthyme · 2021-01-20T15:44:00Z

@sulemanof, @stratoula is right the duplicated legend values are something we are working to fix now.

As for the axes don't have the same labels calculated in split chart mode, we are can open an issue for a secondary axis title in elastic charts. See elastic/elastic-charts#985 and elastic/elastic-charts#986

DianaDerevyankina · 2021-01-20T15:56:22Z

@elasticmachine merge upstream

DianaDerevyankina · 2021-01-21T06:56:11Z

@elasticmachine merge upstream

elasticmachine · 2021-01-25T08:42:43Z

Pinging @elastic/kibana-app (Team:KibanaApp)

stratoula · 2021-01-25T08:43:08Z

@elasticmachine merge upstream

sulemanof

Code LGTM, checked locally in Chrome in Visualize & Dashboard applications, works fine 💞

src/plugins/vis_type_xy/public/chart_splitter.tsx

kibanamachine · 2021-01-25T09:13:02Z

⏳ Build in-progress, with failures

continuous-integration/kibana-ci/pull-request
Commit: 91d5df6
This comment will update when the build is complete

Failed CI Steps

History

💚 Build #100516 succeeded 965824b
💚 Build #100236 succeeded e2f602f
💚 Build #100058 succeeded 282b571
💔 Build #99929 failed ba53abd

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

alexwizp · 2021-01-25T09:23:42Z

@elasticmachine merge upstream

stratoula

LGTM, tested it locally on safari, works fine 🙂 thanx @dziyanadzeraviankina for the functional test!

kibanamachine · 2021-01-25T12:42:36Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 4436b62

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`visTypeXy`	108	107	-1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`visTypeVislib`	643.6KB	643.6KB	-27.0B
`visTypeXy`	157.8KB	160.2KB	+2.4KB
total			+2.4KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`charts`	164.0KB	165.6KB	+1.6KB
`visTypeXy`	69.5KB	62.1KB	-7.4KB
total			-5.7KB

History

💛 Build #100618 was flaky 4cf3bc9
💔 Build #100605 failed 91d5df6
💚 Build #100516 succeeded 965824b
💚 Build #100236 succeeded e2f602f
💚 Build #100058 succeeded 282b571

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* Small multiples in vis_type_xy plugin * Fix tooltip and formatted split chart values * update advanced settings wording * Remove React import in files with no JSX and change the extension to .ts * Simplify conditions * fix bar interval on split charts in vislib * Fix charts not splitting for terms boolean fields * fix filtering for small multiples * Change tests interval values from 100 to 1000000 * Revert "Change tests interval values from 100 to 1000000" This reverts commit 92f9d1b. * Fix tests for interval issue in vislib (cherry picked from commit ef45b63) * Revert axis_scale changes related to interval * Enable _line_chart_split_chart test for new charts library * Move chart splitter id to const Co-authored-by: nickofthyme <nick.ryan.partridge@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* Small multiples in vis_type_xy plugin * Fix tooltip and formatted split chart values * update advanced settings wording * Remove React import in files with no JSX and change the extension to .ts * Simplify conditions * fix bar interval on split charts in vislib * Fix charts not splitting for terms boolean fields * fix filtering for small multiples * Change tests interval values from 100 to 1000000 * Revert "Change tests interval values from 100 to 1000000" This reverts commit 92f9d1b. * Fix tests for interval issue in vislib (cherry picked from commit ef45b63) * Revert axis_scale changes related to interval * Enable _line_chart_split_chart test for new charts library * Move chart splitter id to const Co-authored-by: nickofthyme <nick.ryan.partridge@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: nickofthyme <nick.ryan.partridge@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

DianaDerevyankina and others added 4 commits December 23, 2020 00:24

Small multiples in vis_type_xy plugin

f6b51c5

Fix tooltip and formatted split chart values

645933f

update advanced settings wording

f9ddb84

Merge branch 'master' into Diana/82496

353b3ae

DianaDerevyankina added Feature:XYAxis XY-Axis charts (bar, area, line) Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v7.12.0 labels Dec 23, 2020

DianaDerevyankina requested review from VladLasitsa, nickofthyme, alexwizp and sulemanof December 23, 2020 10:14

DianaDerevyankina self-assigned this Dec 23, 2020

Remove React import in files with no JSX and change the extension to .ts

8f600af

alexwizp removed the v7.11.0 label Dec 29, 2020

alexwizp reviewed Jan 6, 2021

View reviewed changes

alexwizp requested a review from stratoula January 6, 2021 11:29

Merge branch 'master' into Diana/82496

a967804

DianaDerevyankina added 2 commits January 13, 2021 15:00

Merge branch 'master' into Diana/82496

0afd9d5

Simplify conditions

7ba9cea

alexwizp reviewed Jan 13, 2021

View reviewed changes

.ci/packer_cache_for_branch.sh Outdated Show resolved Hide resolved

fix bar interval on split charts in vislib

a7287b9

DianaDerevyankina added 2 commits January 20, 2021 13:57

Merge branch 'master' into Diana/82496

e752837

# Conflicts: # src/plugins/vis_type_xy/public/components/split_chart_warning.tsx # src/plugins/vis_type_xy/public/vis_types/split_tooltip.tsx

Revert axis_scale changes related to interval

ba53abd

alexwizp approved these changes Jan 20, 2021

View reviewed changes

Merge branch 'master' into Diana/82496

282b571

kibanamachine and others added 3 commits January 21, 2021 01:56

Merge branch 'master' into Diana/82496

e2f602f

Merge branch 'master' into Diana/82496

cad1e20

Enable _line_chart_split_chart test for new charts library

965824b

stratoula marked this pull request as ready for review January 25, 2021 08:42

stratoula requested a review from a team January 25, 2021 08:42

Merge branch 'master' into Diana/82496

91d5df6

sulemanof approved these changes Jan 25, 2021

View reviewed changes

src/plugins/vis_type_xy/public/chart_splitter.tsx Outdated Show resolved Hide resolved

Merge branch 'master' into Diana/82496

4cf3bc9

stratoula approved these changes Jan 25, 2021

View reviewed changes

DianaDerevyankina added 2 commits January 25, 2021 13:51

Merge branch 'master' into Diana/82496

48780a0

Move chart splitter id to const

4436b62

DianaDerevyankina merged commit 191ad14 into elastic:master Jan 25, 2021

DianaDerevyankina mentioned this pull request Jan 25, 2021

[7.x] Small multiples in vis_type_xy plugin (#86880) #89127

Merged

wylieconlon mentioned this pull request Jan 29, 2021

[Lens] Support small multiples / facet / trellis for XY and Partition charts #72063

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Small multiples in vis_type_xy plugin #86880

Small multiples in vis_type_xy plugin #86880

DianaDerevyankina commented Dec 23, 2020 •

edited

Loading

alexwizp Jan 6, 2021

alexwizp Jan 6, 2021

stratoula commented Jan 7, 2021

alexwizp commented Jan 13, 2021

stratoula commented Jan 13, 2021

nickofthyme commented Jan 14, 2021

stratoula commented Jan 14, 2021 •

edited

Loading

alexwizp left a comment

sulemanof commented Jan 20, 2021

stratoula commented Jan 20, 2021

nickofthyme commented Jan 20, 2021 •

edited

Loading

DianaDerevyankina commented Jan 20, 2021

DianaDerevyankina commented Jan 21, 2021

elasticmachine commented Jan 25, 2021

stratoula commented Jan 25, 2021

sulemanof left a comment

kibanamachine commented Jan 25, 2021 •

edited

Loading

alexwizp commented Jan 25, 2021

stratoula left a comment

kibanamachine commented Jan 25, 2021

Small multiples in vis_type_xy plugin #86880

Small multiples in vis_type_xy plugin #86880

Conversation

DianaDerevyankina commented Dec 23, 2020 • edited Loading

Summary

Checklist

For maintainers

alexwizp Jan 6, 2021

Choose a reason for hiding this comment

alexwizp Jan 6, 2021

Choose a reason for hiding this comment

stratoula commented Jan 7, 2021

alexwizp commented Jan 13, 2021

stratoula commented Jan 13, 2021

nickofthyme commented Jan 14, 2021

stratoula commented Jan 14, 2021 • edited Loading

alexwizp left a comment

Choose a reason for hiding this comment

sulemanof commented Jan 20, 2021

stratoula commented Jan 20, 2021

nickofthyme commented Jan 20, 2021 • edited Loading

DianaDerevyankina commented Jan 20, 2021

DianaDerevyankina commented Jan 21, 2021

elasticmachine commented Jan 25, 2021

stratoula commented Jan 25, 2021

sulemanof left a comment

Choose a reason for hiding this comment

kibanamachine commented Jan 25, 2021 • edited Loading

⏳ Build in-progress, with failures

Failed CI Steps

History

alexwizp commented Jan 25, 2021

stratoula left a comment

Choose a reason for hiding this comment

kibanamachine commented Jan 25, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

DianaDerevyankina commented Dec 23, 2020 •

edited

Loading

stratoula commented Jan 14, 2021 •

edited

Loading

nickofthyme commented Jan 20, 2021 •

edited

Loading

kibanamachine commented Jan 25, 2021 •

edited

Loading