[TSVB] fix text color when using custom background color by markov00 · Pull Request #60261 · elastic/kibana

markov00 · 2020-03-16T13:46:29Z

Summary

This PR fix the foreground color of the TSVB timeseries chart when the user specify custom background color from the UI.
In particular we are applying the following steps:

if no background color is configured, then apply as usual the chart LIGHT_THEME or DARK_THEME themes
if a background color is configured, then:
1. find the best theme between dark and light that initially satisfy the background luminosity
2. align axis title, labels, ticks and lines to the same color computed, starting from the original colors, to have the best contrast with the current background.
3. apply that theme
4. apply an additional css class that is required to change the legend and no data text colors

fix #60011

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios
This was checked for cross-browser compatibility, including a check against IE11

When the user apply a background color manually from the UI, this commit adapt the current colors to have a better contrast with the choosed background color irrespective of the used dark/light theme

elasticmachine · 2020-03-16T13:47:33Z

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

cchaos · 2020-03-16T14:03:08Z

...egacy/core_plugins/vis_type_timeseries/public/visualizations/views/timeseries/utils/theme.ts

+  bgColor: string,
+  lightFgColor: string,
+  darkFgColor: string,
+  ratio = 4.4


AA contrast is actually 4.5 not 4.4. And this contrast is expected to be attained for text. However, for graphics like axis lines and pure fill colors without text, only a 3.0 contrast level needs to be met.

You are right, I think I've found cases where I cannot reach a 4.5 value with a grey scale foreground color, but maybe I'm completely wrong about that

update here f490b91

cchaos · 2020-03-16T14:08:11Z

...egacy/core_plugins/vis_type_timeseries/public/visualizations/views/timeseries/utils/theme.ts

+  if (relLum <= 0.0031308) {
+    return relLum * 12.92;
+  } else {
+    return (1.0 + 0.055) * Math.pow(relLum, 1.0 / 2.4) - 0.055;


It looks like you are re-writing the WCAG's luminance and contrast calculator in this file. My thoughts are:

A. This would be good to be pulled into a service instead so it doesn't have to be re-written by other chart types or plugins.
B. EUI does export a calculateContrast color service
C. ChromaJS is a dependency and they have a .contrast method

Looks like a good candidate for the “charts” plugin

@cchaos I'm not rewriting the luminance here, I'm inverting the luminance formula to get the lightness to use with the gray color.
This for sure will be part of elastic-charts, I'm currently applying this here because it needs to be released on a patch release and we can't upgrade the chart library in a patch release

I can replace computeContrast with the contrast method of color
We don't have chromajs as a dependency in Kibana right now

updated here: 07f437a

++ for moving this to elastic-charts later on

cchaos · 2020-03-16T14:15:22Z

src/legacy/core_plugins/vis_type_timeseries/public/components/vis_types/_vis_types.scss

+  .tvbVisTimeSeriesDark {
+    .echReactiveChart_unavailable {
+      color: #DFE5EF;
+    }
+    .echLegendItem  {
+      color: #DFE5EF;
+    }
+  }
+  .tvbVisTimeSeriesLight {
+    .echReactiveChart_unavailable {
+      color: #343741;
+    }
+    .echLegendItem  {
+      color: #343741;
+    }
+  }


I could see this becoming an ever-growing list of overrides and we highly suggest not applying straight hex values when they come from the EUI palette/theme. We need to figure out a more extensible approach like possibly adding these to the Chart theme.

This will be moved to elastic-charts for sure

timroes

Tested on Chrome Linux, seems to work with different combinations of darkmode on/off and background colors. One minor nitpick about test names, otherwise this looks good to me.

timroes · 2020-03-16T18:07:18Z

.../core_plugins/vis_type_timeseries/public/visualizations/views/timeseries/utils/theme.test.ts

+import { getTheme } from './theme';
+import { LIGHT_THEME, DARK_THEME } from '@elastic/charts';
+
+describe('src/legacy/core_plugins/vis_type_timeseries/public/visualizations/views/timeseries/utils/theme.ts', () => {


Let's use a proper test name here like "TSVB theme" (even if the rest of TSVB does that weirdly).

updated here 3bd5f54

kibanamachine · 2020-03-17T00:21:08Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 059e34d

History

💔 Build #33534 failed 3bd5f54
💔 Build #33521 failed f490b91
💔 Build #33426 failed bfcecaa

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

* master: (30 commits) [TSVB] fix text color when using custom background color (elastic#60261) Fix import to timefilter from in TSVB (elastic#60296) [NP] Get rid of usage redirectWhenMissing service (elastic#59777) [SIEM] Fix Timeline footer styling (elastic#59587) [ML] Fixes to error handling for analytics jobs and file data viz (elastic#60249) Give better stack traces for Unhandled Promise Rejection warnings (elastic#60235) resolves elastic#58905 (elastic#60120) Added variables button for text fields in Pagerduty component. (elastic#60189) adds test that action vars are rendered for alert action parms (elastic#60310) Closes 59786 by removing the update toast (elastic#60172) [EPM] Packages list tabs (elastic#60167) Added message variables button for Webhook body form field (elastic#60174) Revert "adds new test (elastic#60064)" [Maps] move MapSavedObject type out of telemetry (elastic#60127) [Reporting] Fix error handling for job handler in route (elastic#60161) [Endpoint] TEST: verify alerts page header says 'Alerts' (elastic#60206) EMT-248: implement ack resource to accept event payload to acknowledge agent actions (elastic#60218) Migrate dual validated range (elastic#59689) Embeddable triggers (elastic#58440) [Endpoint] Sample data generator CLI script (elastic#59952) ...

When the user apply a background color manually from the UI, this commit adapt the current colors to have a better contrast with the chosen background color irrespective of the used dark/light theme

…0365) When the user apply a background color manually from the UI, this commit adapt the current colors to have a better contrast with the chosen background color irrespective of the used dark/light theme

…0364) When the user apply a background color manually from the UI, this commit adapt the current colors to have a better contrast with the chosen background color irrespective of the used dark/light theme

* master: (51 commits) do not update cell background if is label cell (elastic#60308) FTR configurable test users (elastic#52431) [Reporting] Wholesale moves client to newest-platform (elastic#58945) [Ingest] Support `show_user` package registry flag (elastic#60338) [SIEM] Adds 'Closes one signal when more than one opened signals are selected' test again (elastic#60380) [SIEM][Detections Engine] - Add rule markdown field to rule create, detail, and edit flows (elastic#60108) [Fleet] Add config revision to fleet agents (elastic#60292) Allow kbn-config-schema to ignore unknown keys (elastic#59560) [ML] Functional tests - disable df analytics clone tests skip flaky suite (elastic#58643) (elastic#58991) [FTR] Add support for --include and --exclude files via tags (elastic#60123) [SIEM] Fix link on overview page (elastic#60348) skip flaky test (elastic#60369) [Endpoint] Adds take action dropdown and tests to alert details flyout (elastic#59242) [Lens] Simplify state management from visualization (elastic#58279) Changing default type to start and allowing it to be configured by the event category (elastic#60323) [ML] Adds the class_assignment_objective to classification (elastic#60358) [TSVB] fix text color when using custom background color (elastic#60261) Fix import to timefilter from in TSVB (elastic#60296) [NP] Get rid of usage redirectWhenMissing service (elastic#59777) ...

* alerting/view-in-app: (53 commits) fixed typo handle optional alerting plugin do not update cell background if is label cell (elastic#60308) FTR configurable test users (elastic#52431) [Reporting] Wholesale moves client to newest-platform (elastic#58945) [Ingest] Support `show_user` package registry flag (elastic#60338) [SIEM] Adds 'Closes one signal when more than one opened signals are selected' test again (elastic#60380) [SIEM][Detections Engine] - Add rule markdown field to rule create, detail, and edit flows (elastic#60108) [Fleet] Add config revision to fleet agents (elastic#60292) Allow kbn-config-schema to ignore unknown keys (elastic#59560) [ML] Functional tests - disable df analytics clone tests skip flaky suite (elastic#58643) (elastic#58991) [FTR] Add support for --include and --exclude files via tags (elastic#60123) [SIEM] Fix link on overview page (elastic#60348) skip flaky test (elastic#60369) [Endpoint] Adds take action dropdown and tests to alert details flyout (elastic#59242) [Lens] Simplify state management from visualization (elastic#58279) Changing default type to start and allowing it to be configured by the event category (elastic#60323) [ML] Adds the class_assignment_objective to classification (elastic#60358) [TSVB] fix text color when using custom background color (elastic#60261) ...

markov00 added 4 commits March 16, 2020 12:49

fix(tsvb): apply better style when using manual bgColor

8ef539a

When the user apply a background color manually from the UI, this commit adapt the current colors to have a better contrast with the choosed background color irrespective of the used dark/light theme

fix(tsvb): add inverted color on no data text

c6a9e01

test: add tests

a24bc8b

Merge branch 'master' into 2020_16_03-fix-tsvb-theme

bfcecaa

markov00 requested a review from timroes March 16, 2020 13:46

markov00 requested a review from a team as a code owner March 16, 2020 13:46

markov00 added release_note:fix bug Fixes for quality problems that affect the customer experience Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.6.2 v8.0.0 labels Mar 16, 2020

cchaos reviewed Mar 16, 2020

View reviewed changes

timroes added the v7.7.0 label Mar 16, 2020

markov00 added 3 commits March 16, 2020 18:27

build: align type dependency to xpack version

5cca603

refactor: use color.contrast instead of using our own function

07f437a

fix: the AA level to 4.5

f490b91

markov00 requested review from cchaos and flash1293 March 16, 2020 17:39

test: rename test

3bd5f54

timroes approved these changes Mar 16, 2020

View reviewed changes

test: fix @elastic/charts mock including themes objects

059e34d

markov00 merged commit e25430b into elastic:master Mar 17, 2020

markov00 mentioned this pull request Mar 17, 2020

[7.x] [TSVB] fix text color when using custom background color (#60261) #60364

Merged

markov00 mentioned this pull request Mar 17, 2020

[7.6] [TSVB] fix text color when using custom background color (#60261) #60365

Merged

This was referenced Mar 25, 2020

Partition: calculate relative luminance for text elastic/elastic-charts#606

Closed

feat: add 4.5 contrast for text in partition slices elastic/elastic-charts#608

Merged

markov00 mentioned this pull request Oct 29, 2020

Legend item text color elastic/elastic-charts#881

Open

Conversation

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

Uh oh!

Summary

Checklist

Uh oh!

elasticmachine commented Mar 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timroes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Mar 17, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

markov00 commented Mar 16, 2020 •

edited

Loading

markov00 Mar 16, 2020 •

edited

Loading