[XY axis] Implement new palette service #86876

stratoula · 2020-12-23T09:39:06Z

Summary

Closes #82495.
This PR integrates the new palette service to the new XY axis plugin. So in order to test it, the Legacy charts library switch should be off (this is the default state).

We introduce a new dropdown on the Panel Settings tab of the XY plugin.

Here the user can choose which palette she wants to use.

For a new visualization the default palette will be chosen.
For an existing visualization the kibana_palette will be chosen. (For this reason, I have added the migration script)

What this PR implements

I have created the color picker component which is exported from the editor plugin as it is going to be used for the rest vislib charts that are going to be converted to elastic charts (pie chart is the next one).
Integration with the new palette service on the xy plugin
Color sync on dashboard (see screenshots below)
Add the dropdown on telemetry to have analytics on the palettes usage (especially how many users are going to use the legacy palette)
Migration script to default the existing visualizations on the kibana palette. (legacy one)

Screenshots

Bar and area charts without color syncing (legacy palette)

Bar and area charts with color syncing (legacy palette)

Color syncing between Lens and XY plugin (default palette)

Checklist

Delete any items that are not applicable to this PR.

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
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

stratoula · 2020-12-23T09:41:38Z

@elasticmachine merge upstream

stratoula · 2020-12-24T07:10:11Z

@elasticmachine merge upstream

stratoula · 2020-12-24T08:11:47Z

@elasticmachine merge upstream

kibanamachine · 2020-12-24T08:11:50Z

merge conflict between base and head

stratoula · 2020-12-29T07:45:24Z

@elasticmachine merge upstream

stratoula · 2021-01-04T06:29:30Z

@elasticmachine merge upstream

stratoula · 2021-01-04T09:13:49Z

src/plugins/vis_type_xy/public/vis_renderer.tsx

    const showNoResult = shouldShowNoResultsMessage(visData, visType);
-    const isSplitChart = Boolean(visConfig.dimensions.splitRow || visConfig.dimensions.splitRow);


This is irrelevant with this PR but it's an OR on the same value.

stratoula · 2021-01-07T08:29:40Z

@elasticmachine merge upstream

flash1293

On editing an existing visualization (I picked one from the logs sample dashboard), it seems like changing the palette doesn't have any effect:

stratoula · 2021-01-07T10:05:51Z

@flash1293 thanx for reviewing and testing this PR 🙂 I will check this!

stratoula · 2021-01-07T11:36:55Z

@flash1293 it should be ok now. Make sure also that the migration script has run. It should default on the legacy palette

stratoula · 2021-01-11T06:39:24Z

@elasticmachine merge upstream

flash1293 · 2021-01-11T11:05:55Z

@stratoula I can still reproduce this specific thing, these are the steps:

Install logs sample data and go to dashboard
Edit "[Logs] Unique Visitors vs. Average Bytes"
Add a split series of terms of geo.src
Change palette
Update
default palette is still used

Maybe it's something about this visualization in particular?

stratoula · 2021-01-11T11:07:07Z

Thanx for the steps Joe, I didn't test this scenario. Let me check it

stratoula · 2021-01-11T11:40:50Z

@flash1293 great catch! It fails for all the visualizations that have more than one metric. I will proceed with a fix

flash1293

Left one remark, but this works pretty well, thanks!

Low prio UI nit - can we make sure the palette picker is full-width here:

That might be a personal thing but it looks weird to me among the full width select elements

flash1293 · 2021-01-11T14:43:22Z

src/plugins/vis_type_xy/public/plugin.ts

@@ -77,8 +77,9 @@ export class VisTypeXyPlugin
    if (!core.uiSettings.get(LEGACY_CHARTS_LIBRARY, false)) {
      setUISettings(core.uiSettings);
      setThemeService(charts.theme);
-      setColorsService(charts.legacyColors);
-
+      charts.palettes.getPalettes().then((palettes) => {


This function should not be called directly in a plugin lifecycle because it's meant to enable lazy loading (if it's loaded every time the plugin is loaded, that's pretty much defeated).

It should be called if it's certain a chart will be rendered very soon (e.g. in the renderer or behind some existing lazy loading barrier)

flash1293

LGTM, works as expected. Thanks for taking care of lazy loading.

We don't necessarily have to change that because I think the experience is nice, but I want to point out one small difference to Lens - if there is no "split series" dimension, it will always use the default palette and don't allow the user to change it.

stratoula · 2021-01-12T10:03:41Z

Thanx Joe 🙂 Yes, I know that is quite different from Lens and I thought about it too but I like it that way. I don't have a strong opinion on it but we can leave it as it is and see how it goes

stratoula · 2021-01-12T10:24:17Z

@elasticmachine merge upstream

nickofthyme

LGTM, great addition! Tested locally on chrome on multiple sample visualizations.

src/plugins/vis_type_xy/public/vis_component.tsx

src/plugins/vis_type_xy/public/vis_renderer.tsx

ppisljar

code LGTM

stratoula · 2021-01-13T11:24:54Z

@elasticmachine merge upstream

kibanamachine · 2021-01-13T13:38:48Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 36b7cd9

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`visDefaultEditor`	126	127	+1
`visTypeXy`	107	108	+1
total			+2

Async chunks

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

id	before	after	diff
`visTypeXy`	152.0KB	157.4KB	+5.4KB

Page load bundle

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

id	before	after	diff
`expressions`	200.6KB	200.7KB	+122.0B
`visDefaultEditor`	44.3KB	46.4KB	+2.0KB
`visTypeXy`	68.0KB	69.5KB	+1.5KB
`visualizations`	171.0KB	171.2KB	+228.0B
total			+3.8KB

History

💚 Build #98481 succeeded 61bb3ea
💚 Build #98175 succeeded 8670de0
💚 Build #97959 succeeded bcfeb66
💚 Build #97887 succeeded 6032e77
💚 Build #97854 succeeded d47d6e9

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

* [XY Axis] New Palette service * Calculate all Series to map the colors correctly * remove commented out code * syncColors on XY plugin * Reset to false when no embeddable * Add unit test for getAllSeries function * Measure the usage of the selected palette * Minor adjustments * Update documentation for isSyncColorsEnabled method * Fix bug on changing palette on charts with no split series * Fix coloring for multiple y axis visualizations * Call getPalettes function from the renderer * Fullwidth palette picker * Fetch palette registry on the component and not on the renderer Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* [XY Axis] New Palette service * Calculate all Series to map the colors correctly * remove commented out code * syncColors on XY plugin * Reset to false when no embeddable * Add unit test for getAllSeries function * Measure the usage of the selected palette * Minor adjustments * Update documentation for isSyncColorsEnabled method * Fix bug on changing palette on charts with no split series * Fix coloring for multiple y axis visualizations * Call getPalettes function from the renderer * Fullwidth palette picker * Fetch palette registry on the component and not on the renderer Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

stratoula added 3 commits December 22, 2020 17:04

[XY Axis] New Palette service

2671aba

Calculate all Series to map the colors correctly

81148ec

remove commented out code

f31ac37

stratoula changed the title ~~Xy plugin new palette~~ [XY axis] Implement new palette service Dec 23, 2020

kibanamachine and others added 3 commits December 23, 2020 04:41

Merge branch 'master' into xy-plugin-new-palette

291f853

syncColors on XY plugin

9064be2

Reset to false when no embeddable

3627a52

kibanamachine and others added 2 commits December 24, 2020 02:10

Merge branch 'master' into xy-plugin-new-palette

9898919

Add unit test for getAllSeries function

cc6152b

stratoula added 2 commits December 24, 2020 10:16

merge with master and resolve conflicts

f94d72c

Measure the usage of the selected palette

a438bbe

Merge branch 'master' into xy-plugin-new-palette

fbce86b

Merge branch 'master' into xy-plugin-new-palette

ff2a600

stratoula commented Jan 4, 2021

View reviewed changes

stratoula added 2 commits January 4, 2021 11:15

Minor adjustments

ed1ac9d

Update documentation for isSyncColorsEnabled method

d3a8dea

stratoula added Feature:XYAxis XY-Axis charts (bar, area, line) v7.12.0 v8.0.0 release_note:enhancement labels Jan 4, 2021

stratoula marked this pull request as ready for review January 4, 2021 13:00

stratoula requested a review from a team January 4, 2021 13:00

stratoula requested a review from a team as a code owner January 4, 2021 13:00

stratoula added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Jan 4, 2021

Merge branch 'master' into xy-plugin-new-palette

6984aa5

flash1293 reviewed Jan 7, 2021

View reviewed changes

Fix bug on changing palette on charts with no split series

2f11f9c

Merge branch 'master' into xy-plugin-new-palette

d47d6e9

Fix coloring for multiple y axis visualizations

6032e77

flash1293 reviewed Jan 11, 2021

View reviewed changes

stratoula added 2 commits January 11, 2021 18:24

Call getPalettes function from the renderer

9124d24

Fullwidth palette picker

bcfeb66

flash1293 approved these changes Jan 12, 2021

View reviewed changes

Merge branch 'master' into xy-plugin-new-palette

8670de0

stratoula mentioned this pull request Jan 12, 2021

[Pie] New implementation of the vislib pie chart with es-charts #83929

Merged

7 tasks

nickofthyme approved these changes Jan 12, 2021

View reviewed changes

src/plugins/vis_type_xy/public/vis_component.tsx Show resolved Hide resolved

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

Fetch palette registry on the component and not on the renderer

61bb3ea

ppisljar approved these changes Jan 13, 2021

View reviewed changes

Merge branch 'master' into xy-plugin-new-palette

36b7cd9

stratoula merged commit 633495e into elastic:master Jan 13, 2021

stratoula mentioned this pull request Jan 13, 2021

[7.x] [XY axis] Implement new palette service (#86876) #88173

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[XY axis] Implement new palette service #86876

[XY axis] Implement new palette service #86876

stratoula commented Dec 23, 2020 •

edited

Loading

stratoula commented Dec 23, 2020

stratoula commented Dec 24, 2020

stratoula commented Dec 24, 2020

kibanamachine commented Dec 24, 2020

stratoula commented Dec 29, 2020

stratoula commented Jan 4, 2021

stratoula Jan 4, 2021

stratoula commented Jan 7, 2021

flash1293 left a comment

stratoula commented Jan 7, 2021

stratoula commented Jan 7, 2021

stratoula commented Jan 11, 2021

flash1293 commented Jan 11, 2021 •

edited

Loading

stratoula commented Jan 11, 2021

stratoula commented Jan 11, 2021

flash1293 left a comment

flash1293 Jan 11, 2021

flash1293 left a comment

stratoula commented Jan 12, 2021

stratoula commented Jan 12, 2021

nickofthyme left a comment

ppisljar left a comment

stratoula commented Jan 13, 2021

kibanamachine commented Jan 13, 2021

		const showNoResult = shouldShowNoResultsMessage(visData, visType);
		const isSplitChart = Boolean(visConfig.dimensions.splitRow \|\| visConfig.dimensions.splitRow);

[XY axis] Implement new palette service #86876

[XY axis] Implement new palette service #86876

Conversation

stratoula commented Dec 23, 2020 • edited Loading

Summary

What this PR implements

Screenshots

Checklist

For maintainers

stratoula commented Dec 23, 2020

stratoula commented Dec 24, 2020

stratoula commented Dec 24, 2020

kibanamachine commented Dec 24, 2020

stratoula commented Dec 29, 2020

stratoula commented Jan 4, 2021

stratoula Jan 4, 2021

Choose a reason for hiding this comment

stratoula commented Jan 7, 2021

flash1293 left a comment

Choose a reason for hiding this comment

stratoula commented Jan 7, 2021

stratoula commented Jan 7, 2021

stratoula commented Jan 11, 2021

flash1293 commented Jan 11, 2021 • edited Loading

stratoula commented Jan 11, 2021

stratoula commented Jan 11, 2021

flash1293 left a comment

Choose a reason for hiding this comment

flash1293 Jan 11, 2021

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

stratoula commented Jan 12, 2021

stratoula commented Jan 12, 2021

nickofthyme left a comment

Choose a reason for hiding this comment

ppisljar left a comment

Choose a reason for hiding this comment

stratoula commented Jan 13, 2021

kibanamachine commented Jan 13, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

stratoula commented Dec 23, 2020 •

edited

Loading

flash1293 commented Jan 11, 2021 •

edited

Loading