[TSVB] Use default Kibana palette for split series #62241

wylieconlon · 2020-04-01T21:59:35Z

Release note

TSVB colors now match other visualizations in dashboards by default.

Before:

After:

Migration note

A migration has been added targeting 7.8.0. The behavior of the migration is to look for TSVB visualizations where the split_color_mode was default, and to explicitly set the value to gradient, which was the previous default. This migration will continue to work in the future because the split_color_mode can no longer be missing.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2020-04-01T21:59:37Z

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

majagrubic

Great change, Wylie 🙇‍♀
I tested this in Chrome on Mac on "ecommerce" index pattern - works as expected. Tested in light and dark mode. Haven't tested in other browsers. Regarding the code itself, I'm not too familiar with color calculations code, but overall looks good to me.

majagrubic · 2020-04-02T10:05:31Z

src/legacy/core_plugins/vis_type_timeseries/public/visualizations/views/timeseries/index.js

@@ -163,6 +169,8 @@ export const TimeSeries = ({
          const stackAccessors = getStackAccessors(stack);
          const isPercentage = stack === STACKED_OPTIONS.PERCENT;
          const key = `${id}-${label}`;
+          // Only use color mapping if there is no color from the server
+          const finalColor = color ?? colors.mappedColors.mapping[label];


Nit: can we use ternary operator here, it's a bit more readable.

flash1293

Tested in chrome and looks much better to me 👍

It's a little strange that the rainbow color theme is now applied on the client while the gradient is applied on the server - could you create a technical debt issue to clean that up in a later iteration? Now it's implicitly using the rainbow theme on the client when the server doesn't provide a color, I can see this causing confusing behavior in a few years from now.

Just a side note: As long as the theme isn't changed in the UI, the TSVB request doesn't contain a value for it - this means we are now implicitly switching a lot of existing TSVB visualizations from gradient to rainbow (if I'm not mistaken). Not sure whether that's intended or not - I have a slight preference for keeping existing visualizations in their current state, but I'm fine with both ways. If that was intended take my approval :)

wylieconlon · 2020-04-02T15:00:18Z

@flash1293 Thanks for the note, that is exactly the kind of case I'm interested in getting feedback on. It sounds like I could fix this by always sending the color, even if it is the default.

flash1293 · 2020-04-02T16:03:23Z

@wylieconlon sounds good to me, I'm not sure how awkward it is to place this in the current code structure.

markov00

I don't want to fully block this PR, but I'd like to highlight the fact that if you have saved a visualization with the default color scheme in TSVB in a previous version, you will be suddenly render a visualization with a completely different set of colors and this, from my point of view is a breaking change.
Let's me show the example: this is a TSVB chart with rainbow colors saved before this PR.

After applying this PR the TSVB chart will render as:

I think this can be considered a visual breaking change.

My suggestion is to add a saved object migration that migrate a default rainbow configuration into a set of custom user configured colors for that visualization to keep the preexisting colors to every preexisting saved visualization. For every new chart is fine to apply the new color scheme

flash1293 · 2020-04-03T14:00:54Z

Good idea with the migration, @markov00 - we can also fix the breaking change switching from gradient to rainbow implicitly I mentioned above the same way - seems like the cleanest option.

I'm not sure whether it's necessary to keep the colors exactly the same for existing visualizations - as there is no explicit mapping from label to color like in standard visualizations, it's anyway not guaranteed to get the same colors for a visualization - e.g. when a filter is active, a color could get mapped to different series any time.

wylieconlon · 2020-04-03T14:25:34Z

@markov00 Echoing something that @flash1293 wrote, I originally did not think a migration would be useful because users have never been able to customize the rainbow palette. I made the choice to put up a PR which replaces the behavior of "rainbow" with different behavior because I think of this as fixing a bug, not as a breaking change.

What you're proposing seems to be that instead of replacing the current behavior, I should add a new behavior. For example:

Kibana palette (new default)
Rainbow
Gradient

This change is possible, but it means that we can't get rid of the existing rainbow palette. Would appreciate some more feedback about whether to continue with the approach in this PR, or to add instead of replacing.

markov00 · 2020-04-03T14:49:12Z

The idea of adding the Kibana Palette as default is a better solution. What if we keep the rainbow one on the dropdown, but make it not selectable from TSVB dropdown.
It's selected by default if you visualization used it before, but if you change it to Kibana or Gradient and save the visualization you can't select it anymore. Only a manual change in the saved object will let the user go back to the original rainbow, or they can reenable it from a kibana.yml configuration.
This basically help us to get rid of all old rainbow palette making it unavailable for new vis, but doesn't create a breaking change for the existing one

markov00 · 2020-04-03T14:52:01Z

Also remember that we should move toward the EUI visualization color palette in 8.0 that is slightly different from the one currently used in kibana

wylieconlon · 2020-04-03T21:44:55Z

@markov00 It's a bit more complicated to hide parts of the UI conditionally, like you have suggested. Can you take another look at this change which adds the new behavior and a migration?

wylieconlon · 2020-04-07T19:29:30Z

@elasticmachine merge upstream

wylieconlon · 2020-04-13T22:13:55Z

@elasticmachine merge upstream

wylieconlon · 2020-04-14T14:43:38Z

@elasticmachine merge upstream

wylieconlon · 2020-04-16T20:15:54Z

src/plugins/visualizations/server/saved_objects/visualization_migrations.ts

+        },
+      };
+    }
+  }


@elastic/kibana-app-arch This migration needs codeowners approval

markov00

LGTM, tested locally and works correctly.
The migration keeps the "old" used default to avoid any color breaking change.

streamich

I just looked at AppArch code changes, approving to unblock. The changes look fine to me, but if you want a thorough review, then maybe @ppisljar could take a look on Monday.

wylieconlon · 2020-04-17T18:37:04Z

Thanks @streamich for the review to unblock! I think this PR has had enough review, but doesn't need to be merged today. So I will wait until Monday evening to merge, giving @ppisljar a chance to review- but I consider it optional.

ppisljar

migration looks good.

wylieconlon · 2020-04-20T14:22:27Z

@elasticmachine merge upstream

kibanamachine · 2020-04-20T16:16:13Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 14f75e3

History

💚 Build #41371 succeeded 3b76922
💚 Build #40672 succeeded 396cb83
💚 Build #40541 succeeded 2b240c8
💚 Build #39414 succeeded ef41763
💔 Build #39078 failed 633e0e0

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

* [TSVB] Rainbow palette shares colors with dashboard * Add migration * Add charts as NP dependency Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* [TSVB] Rainbow palette shares colors with dashboard * Add migration * Add charts as NP dependency Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* master: (30 commits) Migrate vis_type_table to kibana/new platform (elastic#63105) Enable include/exclude in Terms agg for numeric fields (elastic#59425) follow conventions for saved object definitions (elastic#63571) [Docs]Adds saved object key setting to load balancing kib instances (elastic#63935) kbn/config-schema: Consider maybe properties as optional keys in ObjectType (elastic#63838) Fix page layouts, clean up unused code (elastic#63992) [SIEM] Adds recursive exact key checks for validation and formatter [Maps] Migrate remaining maps client files to NP (except routi… (elastic#63859) [Maps] Do not fetch geo_shape from docvalues (elastic#63997) Vega doc changes (elastic#63889) [Metrics UI] Reorganize file layout for Metrics UI (elastic#60049) Add sub-menus to Resolver node (for 75% zoom) (elastic#63476) [FTR] Add test suite metrics tracking/output (elastic#62515) [ML] Fixing single metric viewer page padding (elastic#63839) [Discover] Allow user to generate a report after saving a modified saved search (elastic#63623) [Reporting] Config flag to escape formula CSV values (elastic#63645) [Metrics UI] Remove remaining field filtering (elastic#63398) [Maps] fix date labels (elastic#63909) [TSVB] Use default Kibana palette for split series (elastic#62241) Ingest overview page (elastic#63214) ...

…t-node-pipelines * 'master' of github.com:elastic/kibana: (32 commits) Move authz lib out of snapshot restore (#63947) Migrate vis_type_table to kibana/new platform (#63105) Enable include/exclude in Terms agg for numeric fields (#59425) follow conventions for saved object definitions (#63571) [Docs]Adds saved object key setting to load balancing kib instances (#63935) kbn/config-schema: Consider maybe properties as optional keys in ObjectType (#63838) Fix page layouts, clean up unused code (#63992) [SIEM] Adds recursive exact key checks for validation and formatter [Maps] Migrate remaining maps client files to NP (except routi… (#63859) [Maps] Do not fetch geo_shape from docvalues (#63997) Vega doc changes (#63889) [Metrics UI] Reorganize file layout for Metrics UI (#60049) Add sub-menus to Resolver node (for 75% zoom) (#63476) [FTR] Add test suite metrics tracking/output (#62515) [ML] Fixing single metric viewer page padding (#63839) [Discover] Allow user to generate a report after saving a modified saved search (#63623) [Reporting] Config flag to escape formula CSV values (#63645) [Metrics UI] Remove remaining field filtering (#63398) [Maps] fix date labels (#63909) [TSVB] Use default Kibana palette for split series (#62241) ...

[TSVB] Rainbow palette shares colors with dashboard

2b7c4fd

wylieconlon added release_note:enhancement Feature:TSVB TSVB (Time Series Visual Builder) Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 v7.8.0 labels Apr 1, 2020

wylieconlon requested review from timroes, flash1293 and majagrubic April 1, 2020 21:59

majagrubic approved these changes Apr 2, 2020

View reviewed changes

timroes requested a review from markov00 April 2, 2020 14:35

flash1293 approved these changes Apr 2, 2020

View reviewed changes

markov00 requested changes Apr 3, 2020

View reviewed changes

wylieconlon added 2 commits April 3, 2020 16:15

Merge remote-tracking branch 'origin/master' into tsvb-colors

03caf96

Add migration

30fb476

wylieconlon requested a review from a team as a code owner April 3, 2020 21:39

wylieconlon changed the title ~~[TSVB] Standardize rainbow color palette~~ [TSVB] Use default Kibana palette for split series Apr 3, 2020

Merge remote-tracking branch 'origin/master' into tsvb-colors

633e0e0

wylieconlon requested a review from markov00 April 7, 2020 19:29

Merge branch 'master' into tsvb-colors

ef41763

Merge branch 'master' into tsvb-colors

2b240c8

elasticmachine and others added 3 commits April 14, 2020 10:43

Merge branch 'master' into tsvb-colors

396cb83

Merge remote-tracking branch 'origin/master' into tsvb-colors

745ab14

Add charts as NP dependency

3b76922

wylieconlon requested a review from a team April 16, 2020 20:14

wylieconlon commented Apr 16, 2020

View reviewed changes

wylieconlon requested a review from stacey-gammon April 16, 2020 20:17

markov00 approved these changes Apr 17, 2020

View reviewed changes

streamich approved these changes Apr 17, 2020

View reviewed changes

wylieconlon requested a review from ppisljar April 17, 2020 18:28

ppisljar approved these changes Apr 20, 2020

View reviewed changes

Merge branch 'master' into tsvb-colors

14f75e3

wylieconlon merged commit 7e3de56 into elastic:master Apr 20, 2020

wylieconlon deleted the tsvb-colors branch April 20, 2020 16:43

wylieconlon mentioned this pull request Apr 20, 2020

[7.x] [TSVB] Use default Kibana palette for split series (#62241) #63979

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TSVB] Use default Kibana palette for split series #62241

[TSVB] Use default Kibana palette for split series #62241

wylieconlon commented Apr 1, 2020 •

edited

Loading

elasticmachine commented Apr 1, 2020

majagrubic left a comment •

edited

Loading

majagrubic Apr 2, 2020

flash1293 left a comment

wylieconlon commented Apr 2, 2020 •

edited

Loading

flash1293 commented Apr 2, 2020

markov00 left a comment

flash1293 commented Apr 3, 2020

wylieconlon commented Apr 3, 2020

markov00 commented Apr 3, 2020

markov00 commented Apr 3, 2020

wylieconlon commented Apr 3, 2020

wylieconlon commented Apr 7, 2020

wylieconlon commented Apr 13, 2020

wylieconlon commented Apr 14, 2020

wylieconlon Apr 16, 2020

markov00 left a comment

streamich left a comment

wylieconlon commented Apr 17, 2020

ppisljar left a comment

wylieconlon commented Apr 20, 2020

kibanamachine commented Apr 20, 2020

[TSVB] Use default Kibana palette for split series #62241

[TSVB] Use default Kibana palette for split series #62241

Conversation

wylieconlon commented Apr 1, 2020 • edited Loading

Release note

Migration note

Checklist

elasticmachine commented Apr 1, 2020

majagrubic left a comment • edited Loading

Choose a reason for hiding this comment

majagrubic Apr 2, 2020

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

wylieconlon commented Apr 2, 2020 • edited Loading

flash1293 commented Apr 2, 2020

markov00 left a comment

Choose a reason for hiding this comment

flash1293 commented Apr 3, 2020

wylieconlon commented Apr 3, 2020

markov00 commented Apr 3, 2020

markov00 commented Apr 3, 2020

wylieconlon commented Apr 3, 2020

wylieconlon commented Apr 7, 2020

wylieconlon commented Apr 13, 2020

wylieconlon commented Apr 14, 2020

wylieconlon Apr 16, 2020

Choose a reason for hiding this comment

markov00 left a comment

Choose a reason for hiding this comment

streamich left a comment

Choose a reason for hiding this comment

wylieconlon commented Apr 17, 2020

ppisljar left a comment

Choose a reason for hiding this comment

wylieconlon commented Apr 20, 2020

kibanamachine commented Apr 20, 2020

💚 Build Succeeded

History

wylieconlon commented Apr 1, 2020 •

edited

Loading

majagrubic left a comment •

edited

Loading

wylieconlon commented Apr 2, 2020 •

edited

Loading