Vega vis needs to have visible fonts for dark theme dashboard #73675

VladLasitsa · 2020-07-29T14:39:18Z

Closes #16456

Summary

Added support for dark theme in vega:

Now we have this color behavior on dark mode:

And light theme:

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
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
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

src/plugins/vis_type_vega/public/data_model/vega_parser.ts

VladLasitsa · 2020-07-30T10:40:45Z

@elasticmachine merge upstream

elasticmachine · 2020-07-31T07:27:48Z

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

VladLasitsa · 2020-08-03T09:48:53Z

@elasticmachine merge upstream

timroes · 2020-08-04T09:51:55Z

@elastic/datavis Would you mind giving this a look (and potentially review)? Since we're changing things around chart colors here, I'd like to get your feedback.

markov00

As described in my comment, the gridline is too pronounced and we should introduce/reuse the existing EUI chart theme for that.

Please also check that there are a few missing customizable configurations like the group-title and group-subtitle that can be used and that needs the same theme default as the others

src/plugins/vis_type_vega/public/data_model/vega_parser.ts

VladLasitsa · 2020-08-05T14:04:34Z

@elasticmachine merge upstream

timroes · 2020-08-06T16:39:31Z

We should not inline those color codes in Kibana. If we cannot access them right now in EUI, we need to talk to EUI (or open PR there) to access them. It seems though like at least the createTheme is accessible from Kibana, so we could pull them out there. @markov00 might be able to know more.

VladLasitsa · 2020-08-07T07:58:06Z

We should not inline those color codes in Kibana. If we cannot access them right now in EUI, we need to talk to EUI (or open PR there) to access them. It seems though like at least the createTheme is accessible from Kibana, so we could pull them out there. @markov00 might be able to know more.

I got needed colors. I checked which colors used in chart themes and used it.

timroes · 2020-08-08T12:10:35Z

We should not inline those color codes in Kibana. If we cannot access them right now in EUI, we need to talk to EUI (or open PR there) to access them. It seems though like at least the createTheme is accessible from Kibana, so we could pull them out there. @markov00 might be able to know more.

I got needed colors. I checked which colors used in chart themes and used it.

Yeap that once was on me, sorry. I reviewed on my phone and confused the test files with the source files.

VladLasitsa · 2020-08-10T06:46:09Z

@elasticmachine merge upstream

markov00

I've added one suggestion and requested a slight modification to include subtitle and use the right color for axis title

markov00 · 2020-08-11T08:43:03Z

src/plugins/vis_type_vega/public/data_model/vega_parser.ts

+ this._setDefaultValue(euiThemeVars.euiColorDarkShade, 'config', 'style', 'guide-title', 'fill');
+ this._setDefaultValue(euiThemeVars.euiColorDarkShade, 'config', 'style', 'group-title', 'fill');


Suggested change

this._setDefaultValue(euiThemeVars.euiColorDarkShade, 'config', 'style', 'guide-title', 'fill');

this._setDefaultValue(euiThemeVars.euiColorDarkShade, 'config', 'style', 'group-title', 'fill');

this._setDefaultValue(euiThemeVars.euiColorDarkestShade, 'config', 'style', 'guide-title', 'fill');

this._setDefaultValue(euiThemeVars.euiColorDarkestShade, 'config', 'style', 'group-subtitle', 'fill');

this._setDefaultValue(euiThemeVars.euiColorDarkestShade, 'config', 'style', 'group-title', 'fill');

We should threat both the guide-title (the axis title) and the group-subtitle (used when the user wants a subtitle) with the same color as specified by EUI theme.

you can test the subtitle with this spec on vega:

"title": { "text": "Title Text", "subtitle": "Subtitle Text" }

@VladLasitsa I think there you haven't fixed my suggestion to use euiColorDarkestShade (as in the current EUI theme) for all of the following styles: guide-title, group-subtitle and group-title.

src/plugins/vis_type_vega/public/data_model/vega_parser.test.js

timroes

Tested on Firefox Linux, Light and dark theme on the default Vega spec. Works as intended, Code LGTM

markov00

We should stick exactly to the color used by EUI, please check my comments below

markov00 · 2020-08-13T17:32:59Z

src/plugins/vis_type_vega/public/data_model/vega_parser.ts

+ this._setDefaultValue(euiThemeVars.euiColorDarkShade, 'config', 'style', 'guide-title', 'fill');
+ this._setDefaultValue(euiThemeVars.euiColorDarkShade, 'config', 'style', 'group-title', 'fill');


@VladLasitsa I think there you haven't fixed my suggestion to use euiColorDarkestShade (as in the current EUI theme) for all of the following styles: guide-title, group-subtitle and group-title.

VladLasitsa · 2020-08-15T13:10:35Z

@elasticmachine merge upstream

VladLasitsa · 2020-08-15T13:30:32Z

We should stick exactly to the color used by EUI, please check my comments below

done

markov00

LGTM

…c#73675) * Support dark theme in vega * Fixed tests * Fixed colors * Fixed tests * Fixed comment * Fixed colors Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

kibanamachine · 2020-08-17T14:17:18Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 1722595

Build metrics

async chunks size

id	value	diff	baseline
visTypeVega	1.4MB	+1.1KB	1.4MB

page load bundle size

id	value	diff	baseline
visTypeVega	661.8KB	+65.0B	661.7KB

History

💚 Build #68287 succeeded 1722595
💚 Build #67710 succeeded 8563299
💚 Build #67148 succeeded b4198a0
💚 Build #67102 succeeded 71e7f36
💔 Build #66951 failed d07bb0c

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

#75139) * Support dark theme in vega * Fixed tests * Fixed colors * Fixed tests * Fixed comment * Fixed colors Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Support dark theme in vega

55c393a

VladLasitsa requested a review from timroes July 29, 2020 14:39

timroes reviewed Jul 29, 2020

View reviewed changes

src/plugins/vis_type_vega/public/data_model/vega_parser.ts Outdated Show resolved Hide resolved

Fixed tests

b3e59ad

Merge branch 'master' into elastic#16456

b7a07af

VladLasitsa marked this pull request as ready for review July 31, 2020 07:26

VladLasitsa requested a review from a team July 31, 2020 07:26

VladLasitsa self-assigned this Jul 31, 2020

VladLasitsa added Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.10.0 v8.0.0 labels Jul 31, 2020

Merge branch 'master' into elastic#16456

660ef62

markov00 self-requested a review August 4, 2020 15:43

markov00 requested changes Aug 4, 2020

View reviewed changes

src/plugins/vis_type_vega/public/data_model/vega_parser.ts Outdated Show resolved Hide resolved

elasticmachine and others added 3 commits August 5, 2020 08:04

Merge branch 'master' into elastic#16456

eba4a64

Fixed colors

e4dd402

Merge branch 'master' into elastic#16456

d07bb0c

Fixed tests

71e7f36

Merge branch 'master' into elastic#16456

b4198a0

markov00 reviewed Aug 11, 2020

View reviewed changes

VladLasitsa added 3 commits August 12, 2020 17:34

Fixed comment

35f86f1

Merge remote-tracking branch 'origin/elastic#16456' into elastic#16456

54d3654

Merge remote-tracking branch 'upstream/master' into elastic#16456

8563299

timroes requested a review from markov00 August 13, 2020 08:04

timroes approved these changes Aug 13, 2020

View reviewed changes

markov00 requested changes Aug 13, 2020

View reviewed changes

elasticmachine and others added 2 commits August 15, 2020 07:10

Merge branch 'master' into elastic#16456

5b3ceda

Fixed colors

1722595

markov00 approved these changes Aug 17, 2020

View reviewed changes

VladLasitsa merged commit d5fd34e into elastic:master Aug 17, 2020

VladLasitsa mentioned this pull request Aug 17, 2020

[7.x] Vega vis needs to have visible fonts for dark theme dashboard (#73675) #75139

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vega vis needs to have visible fonts for dark theme dashboard #73675

Vega vis needs to have visible fonts for dark theme dashboard #73675

VladLasitsa commented Jul 29, 2020 •

edited by alexwizp

Loading

VladLasitsa commented Jul 30, 2020

elasticmachine commented Jul 31, 2020

VladLasitsa commented Aug 3, 2020

timroes commented Aug 4, 2020

markov00 left a comment

VladLasitsa commented Aug 5, 2020

timroes commented Aug 6, 2020

VladLasitsa commented Aug 7, 2020

timroes commented Aug 8, 2020

VladLasitsa commented Aug 10, 2020

markov00 left a comment

markov00 Aug 11, 2020

markov00 Aug 11, 2020

VladLasitsa Aug 12, 2020

markov00 Aug 13, 2020

timroes left a comment

markov00 left a comment •

edited by VladLasitsa

Loading

markov00 Aug 13, 2020

VladLasitsa commented Aug 15, 2020

VladLasitsa commented Aug 15, 2020

markov00 left a comment

kibanamachine commented Aug 17, 2020

		this._setDefaultValue(euiThemeVars.euiColorDarkShade, 'config', 'style', 'guide-title', 'fill');
		this._setDefaultValue(euiThemeVars.euiColorDarkShade, 'config', 'style', 'group-title', 'fill');

- this._setDefaultValue(euiThemeVars.euiColorDarkShade, 'config', 'style', 'guide-title', 'fill');
- this._setDefaultValue(euiThemeVars.euiColorDarkShade, 'config', 'style', 'group-title', 'fill');
+ this._setDefaultValue(euiThemeVars.euiColorDarkestShade, 'config', 'style', 'guide-title', 'fill');
+ this._setDefaultValue(euiThemeVars.euiColorDarkestShade, 'config', 'style', 'group-subtitle', 'fill');
+ this._setDefaultValue(euiThemeVars.euiColorDarkestShade, 'config', 'style', 'group-title', 'fill');

Vega vis needs to have visible fonts for dark theme dashboard #73675

Vega vis needs to have visible fonts for dark theme dashboard #73675

Conversation

VladLasitsa commented Jul 29, 2020 • edited by alexwizp Loading

Summary

Checklist

For maintainers

VladLasitsa commented Jul 30, 2020

elasticmachine commented Jul 31, 2020

VladLasitsa commented Aug 3, 2020

timroes commented Aug 4, 2020

markov00 left a comment

Choose a reason for hiding this comment

VladLasitsa commented Aug 5, 2020

timroes commented Aug 6, 2020

VladLasitsa commented Aug 7, 2020

timroes commented Aug 8, 2020

VladLasitsa commented Aug 10, 2020

markov00 left a comment

Choose a reason for hiding this comment

markov00 Aug 11, 2020

Choose a reason for hiding this comment

markov00 Aug 11, 2020

Choose a reason for hiding this comment

VladLasitsa Aug 12, 2020

Choose a reason for hiding this comment

markov00 Aug 13, 2020

Choose a reason for hiding this comment

timroes left a comment

Choose a reason for hiding this comment

markov00 left a comment • edited by VladLasitsa Loading

Choose a reason for hiding this comment

markov00 Aug 13, 2020

Choose a reason for hiding this comment

VladLasitsa commented Aug 15, 2020

VladLasitsa commented Aug 15, 2020

markov00 left a comment

Choose a reason for hiding this comment

kibanamachine commented Aug 17, 2020

💚 Build Succeeded

Build metrics

async chunks size

page load bundle size

History

VladLasitsa commented Jul 29, 2020 •

edited by alexwizp

Loading

markov00 left a comment •

edited by VladLasitsa

Loading