fix(css): remove dependency on EUI components and use only EUI styles #208

markov00 · 2019-05-20T21:12:26Z

Summary

This PR update the EUI dependency and clean its use inside the library. The following thing are changed:

each css style is prefixed with ech instead of the long elasticCharts
the new css is fully compatible with IE11, Chrome and Firefox
the legend toggle is currently commented out ( we can decide to reintroduce it or leave that to the external container)
the color picker and the contextual menu on the legend are temporarily disabled. They are actually part of EUI, so we can have add them as external prop components in a later version (only vislib is currently use this function)
the style for the eye open and closed on the legend is different from the EUI one: there is no animation on that button, but only a static change.

Before

After

text are a bit more condensed and the width of the legend is changed from 160px to 200px.

Before

After

fixed the alignment of latest element on bottom/top legend

Before

After

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
This was checked for cross-browser compatibility, including a check against IE11
Proper documentation or storybook story was added for features that require explanation or tutorials
Unit tests were updated or added to match the most common scenarios
Each commit follows the convention

codecov-io · 2019-05-20T21:52:42Z

Codecov Report

Merging #208 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #208      +/-   ##
==========================================
+ Coverage   97.82%   97.82%   +<.01%     
==========================================
  Files          35       36       +1     
  Lines        2569     2571       +2     
  Branches      565      578      +13     
==========================================
+ Hits         2513     2515       +2     
  Misses         49       49              
  Partials        7        7

Impacted Files	Coverage Δ
src/state/utils.ts	`95.58% <ø> (-0.28%)`	⬇️
src/state/crosshair_utils.ts	`85.29% <100%> (ø)`	⬆️
src/lib/series/legend.ts	`100% <100%> (ø)`	⬆️
src/lib/series/series.ts	`99.22% <100%> (+0.03%)`	⬆️
src/lib/themes/colors.ts	`100% <100%> (ø)`
src/lib/themes/light_theme.ts	`100% <100%> (ø)`	⬆️
src/state/chart_state.ts	`97.04% <100%> (-0.04%)`	⬇️
src/lib/series/rendering.ts	`97.94% <100%> (+0.11%)`	⬆️
src/lib/themes/dark_theme.ts	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd02b52...d1b25d8. Read the comment docs.

emmacunningham

just a few small comments but otherwise lgtm. tested locally and looking good!

src/components/icon.tsx

stories/bar_chart.tsx

src/components/icons/assets/alert.tsx

cchaos · 2019-05-21T17:26:39Z

I am struggling to test this in Kibana. I have built it with yarn build then linked it over in Kibana. I see both the theme_light.css and theme_dark.css in node_modules. But when I add those theme files to https://github.com/elastic/kibana/blob/cc7a6ec2c11b51bbbc64a319b29750232a5dc175/src/legacy/ui/ui_render/ui_render_mixin.js#L119-L125

like so:

darkMode ?
              [
                `${basePath}/node_modules/@elastic/eui/dist/eui_theme_dark.css`,
                `${basePath}/node_modules/@kbn/ui-framework/dist/kui_dark.css`,
                `${basePath}/node_modules/@elastic/charts/dist/theme_dark.css`,
              ] : [
                `${basePath}/node_modules/@elastic/eui/dist/eui_theme_light.css`,
                `${basePath}/node_modules/@kbn/ui-framework/dist/kui_light.css`,
                `${basePath}/node_modules/@elastic/charts/dist/theme_light.css`,
              ]

I get 404 errors and it completely borks Kibana, getting the "Kibana didn't load properly" error.

I can import the theme_light.scss file into https://github.com/elastic/kibana/blob/master/src/legacy/ui/public/styles/_styling_constants.scss via:

@import '@elastic/charts/src/theme_light';

But of course, that duplicates these styles numerous times and I'm pretty sure it doesn't swap light for dark.

Any thoughts on how to appropriately handle the themes and importing?

markov00 · 2019-05-21T17:46:49Z

@cchaos I think you have to add the following:

  server.exposeStaticDir('/node_modules/@elastic/charts/dist/{path*}', fromRoot('node_modules/@elastic/charts/dist'));

at line 59 of ui_render_mixin https://github.com/elastic/kibana/blob/cc7a6ec2c11b51bbbc64a319b29750232a5dc175/src/legacy/ui/ui_render/ui_render_mixin.js#L59

I've just tried and it works

src/components/_tooltip.scss

cchaos · 2019-05-21T18:41:23Z

src/style/globals/variables/_typography.scss

+
+// Families
+
+$echFontFamily: 'Inter UI', -apple-system, BlinkMacSystemFont, 'Segoe UI', Helvetica, Arial,


We shouldn't be importing 'Inter UI' in this base library. We should probably just stick to system fonts. In fact, I think there's a lot of unnecessary invisibles brought in here from EUI.

@cchaos what are the system fonts i can use? -apple-system, BlinkMacSystemFont, Helvetica, Arial, sans-serif ?
I also need the right one to use on the canvas text elements, what I can use there?

Removing the Inter UI bit is all you need. The ones that follow are "system fonts". Apple, then MS, then Linux, then Helvetica.

markov00 · 2019-06-04T07:40:15Z

Hey @cchaos are you still working on that? Should we start to wrap up and merge this PR?
If you are working on that, please could you list here what are your current task so I can help you on those?

cchaos · 2019-06-04T15:12:00Z

@markov00 Yes, I'm actively working on this. I think I'm just about done, but have I'm double-checking it in EUI right now and will need to do so in Kibana as well.

cchaos · 2019-06-04T16:45:45Z

@markov00 PR4U: markov00#2

markov00 · 2019-06-05T13:42:48Z

hey @cchaos I've merged your PR and rebased everything.
What do you think about creating two different css output, one with reset and one without.
So that the user just need to import one or the other css, and not two separate files.
I'm also wondering if we need to apply the reset only under the ech container, and not to the whole html. I don't want to mess with the existing reset, but maybe there is something I'm not considering.

markov00 · 2019-06-05T14:00:58Z

@cchaos @snide I've also minor concern on the readability of the highlighted value on the tooltip.

This is the current implementation:

This is the Caroline changes:

From my point of view, adding changing the font-weight with a higher value create few disadvantages:

the highlighted rows is poorly distinguishable from others (your eyes need to catch a small difference in a font size, than in a background color change) specially if you enter quickly into the chart.
changing the font-weight to a higher value increase also the tooltip width creating a strange effect that grow and shrink the tooltip when highlighting a bar.

cchaos · 2019-06-05T14:09:48Z

@markov00 I actually agree. I wanted to come back and revisit that but had forgotten. I will work on those states.

nickofthyme · 2019-06-05T13:44:03Z

package.json

@@ -91,7 +90,6 @@
    "husky": "^1.3.1",
    "jest": "^24.1.0",
    "jest-environment-jsdom-fourteen": "^0.1.0",
-    "moment": "^2.24.0",


So moment was a dependency of @elastic/[email protected] but not @elastic/eui@^11.2.1?

EUI is only being consumed for it's SASS variables so there's no need fo this peer dependency.

@cchaos cool thanks!

nickofthyme · 2019-06-05T13:44:29Z

package.json

@@ -119,7 +117,6 @@
    "d3-shape": "^1.3.4",
    "fp-ts": "^1.14.2",
    "konva": "^2.6.0",
-    "lodash": "^4.17.11",


Nice!! 🎉

cchaos · 2019-06-05T16:59:13Z

Pushed a fix for the tooltip:

cchaos · 2019-06-05T17:02:32Z

I'm also wondering if we need to apply the reset only under the ech container, and not to the whole html. I don't want to mess with the existing reset, but maybe there is something I'm not considering.

The problem with that is that it will override any base styles declared outside of the .echContainer.

Example

Reset file includes:

a { color: blue }

Consumer's theme is:

a { color: red }

But then including the charts styles with the reset would make it:

.echContainer a { color: blue }

completely overriding the consumer's theme.

cchaos · 2019-06-05T17:36:40Z

@markov00 @snide My latest commit does as @markov00 had asked and compiles one file with reset and theme, and then a theme only file.

snide · 2019-06-05T17:40:35Z

@cchaos OK. Looks good. Thanks for updating the docs too.

markov00 · 2019-06-06T16:59:45Z

@cchaos I've added the legend toggle button, Do you want to take a look if everything seems fine?

I've also played a bit with the dark mode, and seems that in darkmode the tooltip is fully black. if we have a full black 000 background the tooltip can disappear easily:

This PR:

On a chart with #000 background

Current implementation:

cchaos · 2019-06-06T17:46:03Z

I've also played a bit with the dark mode, and seems that in darkmode the tooltip is fully black.

This is how the tooltip works in EUI. Not backgrounds should actually be fully black. Our default "white" background actually goes to a "dark gray" in dark mode:

I had also changed that fully black background to our default background color here: https://github.com/elastic/elastic-charts/pull/208/files#diff-6cc5987d83a2bd4599131f08c70199cf

You can see there is a slight variation in color. But that's how we work with them currently in EUI and would like to stick with the same method here.

cchaos · 2019-06-06T17:58:41Z

Pushed more fixes to legend toggle.

markov00 · 2019-06-07T08:09:12Z

@cchaos I'm ok with the changes. If you think we are done, I will fix the squash commit and merge it

cchaos

I'm happy with it. We'll want to setup a PR to update it in Kibana correctly with the new styles imports.

markov00 · 2019-06-07T14:28:37Z

@cchaos I can do that if you want, let me fix conflict, merge and I will create the PR in Kibana

Introduced new chart scss code to replace the existing EUI sass styles.

All class and mixins are now under .ech prefix

This commit change the current output style to two different styles, one for dark and one for light theme. BREAKING CHANGE: EUI is removed from this library. The single chart `style.css` stylesheet is now replaced by a `theme_light.css` or `theme_dark.css` file that brings in all the required styling for chart, tooltip and legends.

Reverting the EUI removal

the time-scale-utils file was to test some assumptions but it's currently not required. The same for the color functions

# [5.0.0](v4.2.9...v5.0.0) (2019-06-10) ### Bug Fixes * **css:** remove dependency on EUI components and use only EUI styles ([#208](#208)) ([122fade](122fade)) ### BREAKING CHANGES * **css:** EUI components are removed from this library. The single chart `style.css` stylesheet is now replaced by a `theme_only_light.css` or `theme_only_dark.css` file that brings in all the required styling for chart, tooltip and legends. `theme_light.css` and `theme_dark.css` styles include also a reset CSS style

markov00 · 2019-06-10T10:25:47Z

🎉 This PR is included in version 5.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [5.0.0](elastic/elastic-charts@v4.2.9...v5.0.0) (2019-06-10) ### Bug Fixes * **css:** remove dependency on EUI components and use only EUI styles ([opensearch-project#208](elastic/elastic-charts#208)) ([a26d2db](elastic/elastic-charts@a26d2db)) ### BREAKING CHANGES * **css:** EUI components are removed from this library. The single chart `style.css` stylesheet is now replaced by a `theme_only_light.css` or `theme_only_dark.css` file that brings in all the required styling for chart, tooltip and legends. `theme_light.css` and `theme_dark.css` styles include also a reset CSS style

markov00 requested review from emmacunningham, snide and chandlerprall May 20, 2019 21:12

markov00 requested a review from cchaos May 20, 2019 21:53

emmacunningham approved these changes May 21, 2019

View reviewed changes

src/components/icon.tsx Outdated Show resolved Hide resolved

stories/bar_chart.tsx Outdated Show resolved Hide resolved

src/components/icons/assets/alert.tsx Outdated Show resolved Hide resolved

cchaos reviewed May 21, 2019

View reviewed changes

markov00 force-pushed the remove_eui branch from 4e75877 to 9755663 Compare May 22, 2019 09:14

emmacunningham mentioned this pull request Jun 5, 2019

feat: add isChartEmpty #219

Merged

3 tasks

markov00 force-pushed the remove_eui branch from 3f4faaa to 60aa8c0 Compare June 5, 2019 13:33

markov00 changed the title ~~fix: remove EUI dependency~~ fix(css): apply EUI styles without interfere with external css Jun 5, 2019

nickofthyme reviewed Jun 5, 2019

View reviewed changes

cchaos force-pushed the remove_eui branch from a939cf9 to 2bc8e1f Compare June 5, 2019 17:38

markov00 changed the title ~~fix(css): apply EUI styles without interfere with external css~~ fix(css): remove dependency on EUI components and use only EUI styles Jun 6, 2019

markov00 mentioned this pull request Jun 6, 2019

Color Picker functionality cannot be disabled #198

Closed

cchaos approved these changes Jun 7, 2019

View reviewed changes

markov00 and others added 16 commits June 7, 2019 18:17

fix(eui): removed eui dependency

499326e

Introduced new chart scss code to replace the existing EUI sass styles.

fix(eui): migrate eui prefix to ech

89679aa

All class and mixins are now under .ech prefix

refactor: update container class name

f842788

build(deps): remove unused dependencies

d466679

refactor: removed unused icon and fixed wrong props and component

e7c8fa6

fix: fix tooltip dropshadow and remove font family

f0c75f6

fix: move functions to avoid circular dependencies

71f3d95

revert(eui): use EUI strictly as Dev dependency

cebc470

Reverting the EUI removal

fix: tooltip highlighted row

99572b8

fix: compile reset into theme file with separate theme_only file

3fe6339

refactor: remove unused code

bccd5c6

the time-scale-utils file was to test some assumptions but it's currently not required. The same for the color functions

feat(legend): add legend toggle

e894743

style: clean toggle legend style

ddee798

style: fix up legend toggle

21bb075

build: fix package.json peer deps

d1b25d8

markov00 force-pushed the remove_eui branch from d3dadb2 to d1b25d8 Compare June 10, 2019 08:59

markov00 merged commit 122fade into elastic:master Jun 10, 2019

markov00 added the released Issue released publicly label Jun 10, 2019

markov00 mentioned this pull request Jun 10, 2019

The Legend is not displayed correctly in the 'Bottom' mode #194

Closed

markov00 deleted the remove_eui branch December 13, 2019 11:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(css): remove dependency on EUI components and use only EUI styles #208

fix(css): remove dependency on EUI components and use only EUI styles #208

markov00 commented May 20, 2019 •

edited

Loading

codecov-io commented May 20, 2019 •

edited

Loading

emmacunningham left a comment

cchaos commented May 21, 2019

markov00 commented May 21, 2019 •

edited

Loading

cchaos May 21, 2019

markov00 May 22, 2019

snide May 23, 2019

markov00 commented Jun 4, 2019

cchaos commented Jun 4, 2019

cchaos commented Jun 4, 2019

markov00 commented Jun 5, 2019

markov00 commented Jun 5, 2019

cchaos commented Jun 5, 2019

nickofthyme Jun 5, 2019

cchaos Jun 5, 2019

nickofthyme Jun 5, 2019

nickofthyme Jun 5, 2019

cchaos commented Jun 5, 2019

cchaos commented Jun 5, 2019

cchaos commented Jun 5, 2019

snide commented Jun 5, 2019

markov00 commented Jun 6, 2019

cchaos commented Jun 6, 2019

cchaos commented Jun 6, 2019

markov00 commented Jun 7, 2019

cchaos left a comment

markov00 commented Jun 7, 2019

markov00 commented Jun 10, 2019


		// Families

		$echFontFamily: 'Inter UI', -apple-system, BlinkMacSystemFont, 'Segoe UI', Helvetica, Arial,

fix(css): remove dependency on EUI components and use only EUI styles #208

fix(css): remove dependency on EUI components and use only EUI styles #208

Conversation

markov00 commented May 20, 2019 • edited Loading

Summary

Checklist

codecov-io commented May 20, 2019 • edited Loading

Codecov Report

emmacunningham left a comment

Choose a reason for hiding this comment

cchaos commented May 21, 2019

markov00 commented May 21, 2019 • edited Loading

cchaos May 21, 2019

Choose a reason for hiding this comment

markov00 May 22, 2019

Choose a reason for hiding this comment

snide May 23, 2019

Choose a reason for hiding this comment

markov00 commented Jun 4, 2019

cchaos commented Jun 4, 2019

cchaos commented Jun 4, 2019

markov00 commented Jun 5, 2019

markov00 commented Jun 5, 2019

cchaos commented Jun 5, 2019

nickofthyme Jun 5, 2019

Choose a reason for hiding this comment

cchaos Jun 5, 2019

Choose a reason for hiding this comment

nickofthyme Jun 5, 2019

Choose a reason for hiding this comment

nickofthyme Jun 5, 2019

Choose a reason for hiding this comment

cchaos commented Jun 5, 2019

cchaos commented Jun 5, 2019

Example

cchaos commented Jun 5, 2019

snide commented Jun 5, 2019

markov00 commented Jun 6, 2019

cchaos commented Jun 6, 2019

cchaos commented Jun 6, 2019

markov00 commented Jun 7, 2019

cchaos left a comment

Choose a reason for hiding this comment

markov00 commented Jun 7, 2019

markov00 commented Jun 10, 2019

markov00 commented May 20, 2019 •

edited

Loading

codecov-io commented May 20, 2019 •

edited

Loading

markov00 commented May 21, 2019 •

edited

Loading