elastic / elastic-charts

https://elastic.github.io/elastic-charts/storybook
Other
370 stars 120 forks source link

Move EUI chart theme into elastic-charts #865

Closed markov00 closed 2 weeks ago

markov00 commented 4 years ago

Is your feature request related to a problem? Please describe. The elastic-charts theme, used in Kibana, is currently developed and published as part of the EUI library. We ship elastic-charts with a default theme that is not intended to be used inside Kibana.

Describe the solution you'd like

I believe that shipping elastic-charts with the EUI good default theme could be a better option for the following reasons:

The drawback I can identify are:

I'd like to open a discussion on that to understand if there are other drawbacks I haven't considered yet and if that change can be considered applicable by the EUI team.

Describe alternatives you've considered

In Kibana we have a set of utilities to pick up the right theme to use depending on the color mode (dark/light), but this is not used on every chart in Kibana and for sure not used for charts outside Kibana (if any).

Another option is to automatically extract a copy of the themes from the EUI package and expose them as part of the compiled sources of elastic-charts. This involves some scripting but can leave the EUI team the flexibility to work from their own repo.

cc @cchaos @chandlerprall

cchaos commented 4 years ago

Hey @markov00 and Charts team,

This is a very similar discussion we had in the early days of this repo. We had settled on the current structure of keeping Charts and EUI separate in order to best maintain circular dependencies and possibly out-of-sync versioning. I think this is still the best approach, but we're definitely willing to help the Charts team develop a nice looking default theme.

This is how I see the 3 scenarios of how Elastic Charts and EUI can be consumed today:

Screen Shot 2020-10-13 at 15 31 42 PM

Whimsical

Essentially, consumers of Elastic Charts aren't always going to be consuming EUI. Therefore, it only makes sense to consume the 4 theme files if using EUI and by EUI.

If Elastic Charts were to instead own a theme for every EUI theme, you would need to produce, compile and maintain a new theme file for every EUI theme. It also then makes non-consumers of EUI dependent on EUI.

Screen Shot 2020-10-13 at 15 38 49 PM

Whimsical

Which, by reading this comment:

The design team, when need to update/change the current theme setup, has to work on a different repo

would require EUI to "own" code in a different repo. We've tried very hard to keep this from happening and would like to continue to avoid this.


So, what it really sounds to me is that your main ask is:

a consumer doesn't need to import the EUI library at all to have a good looking chart

And I think the best way to solve this is just to get the default theme to have better defaults. The EUI theme does more than just handle the replacement of colors. It adjusts default fonts, font sizes, borders, visibility of elements, etc. These are just design decisions that aren't dependent on EUI specifically.

If you also consider what the EUI and Charts libraries are. Elastic UI is an Elastic branded and owned UI library just like Elastic Charts is an Elastic branded and owned charting library. EUI just happens to be the place devs go for branding consistency. So, I think it makes sense the Elastic Charts uses Elastic branding in the default theme.

Screen Shot 2020-10-13 at 15 55 28 PM

Whimsical

We (EUI designers) can help you build a good-looking default theme, but it doesn't need to be dependent on EUI specifically, you just need the brand elements.


I do understand that we (EUI) have not been great at keeping our theme file and therefore version of Charts up to date. There were talks a while back about how to make this a more automated process. I think this might be the time to make that discussion a priority. I'll also now defer to @chandlerprall on the technicalities of dependencies and how could better support the circular nature.

miukimiu commented 4 years ago

We (EUI designers) can help you build a good-looking default theme, but it doesn't need to be dependent on EUI specifically, you just need the brand elements.

@cchaos, if we go in this direction should EUI have a docs section for Elastic Charts? One thing we talked about during the meetings is to start experimenting with Elastic Docs.

Another solution could be, and we would definitely need @chandlerprall for this, is to start transforming EUI into a monorepo. And the theme(s) would be a package. This already mentioned in our roadmap https://github.com/elastic/eui/issues/2520 and I guess it's going to take a while.

But I would envision something like using Lerna to manage the monorepo and make the theme(s) a package.

Screenshot 2020-10-14 at 16 39 31

if we need to publish a breaking change on the Theme API, we need to sync the dependency upgrade in Kibana with the upgrade of EUI

The sync would be easier because we don't often change the theme(s).

chandlerprall commented 4 years ago

Another solution could be, and we would definitely need @chandlerprall for this, is to start transforming EUI into a monorepo. And the theme(s) would be a package. This already mentioned in our roadmap elastic/eui#2520 and I guess it's going to take a while.

That's the direction I was thinking. Splitting EUI into a monorepo, even at a very high level like src | docs | chart theme should help a lot. We could go further and provide color codes as a separate package which elastic charts could use to build their base theme.

markov00 commented 4 years ago

Thank everyone for the comments, I think the @miukimiu @chandlerprall idea on having a mono-repo with a theme package separate from the current components one is a great idea and can solve cleanly most issues we have, in this way we can ship the Elastic-Charts lib with EUI theme package as a dependency or peer dependency that provides all the necessary chart themes we want. Users of the library inside or outside Elastic will receive the well thought EUI theme by default without the needs of the full EUI library.

@cchaos

We (EUI designers) can help you build a good-looking default theme, but it doesn't need to be dependent on EUI specifically, you just need the brand elements.

That will be great, if you are fine, we can also just copy over the current dark/light themes without any efforts from your theme.