Adding docs for elastic-charts by cchaos · Pull Request #2209 · elastic/eui

cchaos · 2019-08-08T02:55:17Z

This PR will add docs pages for 4 main topics surrounding elastic-charts (and charts in general).

The sections will show the ideal version of the specified chart type. They talk more about how to represent your data correctly than how elastic-charts works. That's what their docs are for. However, it does also provide code to produce the resulting chart so it's easy for consumers to copy/paste these ideal charts.

1. Using elastic-charts and EUI together

Main topic is how to use the EUI produced themes that work with elastic-charts and then a broader topic on best uses for color when creating charts.

2. Time series charts

This is probably the most used chart type in Kibana and will then probably be the most consumed version of elastic-charts.

3. Categorical charts

This is a type of chart that usually gets misrepresented because consumers don't adjust their data properly.

4. Sizing/Sparklines

There isn't always room to show all the details of a chart depending on the size of the chart itself. The sizing section showcases some examples.

Most of the issues that stem around the creation of sparklines is forgetting to remove interactions and extraneous markings.

Checklist

Checked in dark mode
Checked in mobile
Checked in IE11 and Firefox
[ ] Props have proper autodocs
Added documentation examples
[ ] Added or updated jest tests
~~[ ] Checked for breaking changes and labeled appropriately~~
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

markov00 · 2019-08-08T09:08:00Z

Thanks @cchaos for this, it looks awesome.

I have some small comments on the code, but I think I can go through them and fix them directly, like some code strings instead of the Position object, some snippet code has the xScaleType on the Axis.

I think we should also include an example to hide the markers on the line if the line contains many points (100 or more).

My last remark: the font size seems very small for all the labels on the chart, don't you think? Compared to the basic font size (14px)?

cchaos · 2019-08-12T16:10:17Z

Thanks @markov00. Yeah I went through a pass to clean up those extra objects but must have missed some.

I think we should also include an example to hide the markers on the line if the line contains many points (100 or more).

Do you think this should just be a part of elastic-charts? Something it does automatically so that the marker only exists on hover?

the font size seems very small for all the labels on the chart, don't you think?

Agreed. I originally set it that small to sort of get them out of the way because charts should just be a quick glance of noticing trends and differences and then the user should rely on the tooltips for actual values.

The current problem right now is that elastic-charts doesn't allow for good spacing between the tick labels causing a jam up of text.

So reducing the font size helped to keep the user from getting distracted by these values. I do realize, however, that the font size is too small for categorical charts where reading the category names are important for that glance. And there still needs to be better vertical spacing.

Compared to the basic font size (14px)?

The base font size for charts has never been as large as 14px.

But I'm thinking a good compromise is increasing both ticks and labels by just a couple pixels.

snide

It's hard to test this without the theming system being released. I've done some review with @cchaos in zoom's and my general comments were as follows:

Label sizing was likely a tad too small
We will need some sort of guidance around chart sizing. AKA, what makes a good panel view chart and what should and should not be dropping in those sorts of views. Then, what makes a good sparkline / tiny chart. Those three usages are going to be the most common.
Possibly some more instruction on the palette services (chroma) being used. Just something that says... to build charts that look like this. Import this theme, use this service, and then use this chart configuration. Some of that is lost in the doc examples at the moment.

My comments are more toward the charting system in general. Right now it seems it doesn't do well scaling detail down based upon the containers it sits within. I think that's something we need to focus on in general since it's blocking our ability to deliver reliable, adaptive solutions to folks.

I think that outside of the theme service this content is all just documentation so we should be a little more open with just merging this and seeing how it helps, it's something that we can iterate on over time.

markov00 · 2019-08-13T08:36:04Z

@cchaos

Do you think this should just be a part of elastic-charts? Something it does automatically so that the marker only exists on hover?

Do we want to hardcode that max number of markers on a specific size or do we want to add that to the theme itself? I think the last should provide all a better configurability of the chart

For the axis labels: we have a ticket to fix too many labels specifying the number of ticks for each axis.
You can also use the padding style of the tickLabelStyle to allow more space between labels

cchaos · 2019-08-14T14:27:03Z

This PR is now ready for review 🎉

cchaos · 2019-08-14T19:15:47Z

@markov00

Do we want to hardcode that max number of markers on a specific size or do we want to add that to the theme itself? I think the last should provide all a better configurability of the chart

I was thinking that since the chart is already doing all that math and plotting of points that it should be able to determine a minimum distance between points and if that minimum is not met, it would turn off the points. Of course this could be a boolean style prop or setting that just turns the logic on or off but is on by default.

I'm not sure any other good way to do it through the theme (at least with the current configurations that are available now).

cchaos · 2019-08-19T19:58:59Z

@snide

Possibly some more instruction on the palette services (chroma) being used. Just something that says... to build charts that look like this. Import this theme, use this service, and then use this chart configuration. Some of that is lost in the doc examples at the moment.

This is currently not the final way I'd like to be able to support more color schemes/palettes. We can either provide a better service with our own functions (pulling the chroma one out of docs) on top of our named palettes. Or Elastic-charts needs to create a color service for creating palettes.

Either way, there's no good way to describe the way I'm creating these extra palettes at the moment. My biggest goal right now is to get the base EUI chart themes into Kibana so that all the charts can start to look a bit better. I don't think many are using custom color schemes, or if they are they're already providing these hex values directly.

snide · 2019-08-19T21:15:55Z

Yes. I agree. It should be handled on the ECharts side.

thompsongl · 2019-08-19T21:22:30Z

The panels and configuration toggles with built-in best practices is a very cool and very helpful way to do this.
These look to be some of the best examples we have

snide

Lots of small docs changes. Mostly to change to active voice / fix typos (which coming from me you might want to double check). Kind of a big question about the delivery of the themes, but I think the resolution should be pretty simple. We should check with the engineers.

Overall. This is fantastic and what we should strive to do when providing usage documentation for our design work. Nice work. 🥇

src-docs/src/views/elastic_charts/category_chart.js

src-docs/src/views/elastic_charts/category_example.js

src-docs/src/views/elastic_charts/theming_example.js

src-docs/src/views/elastic_charts/time_example.js

src-docs/src/views/elastic_charts/time_chart.js

src-docs/src/views/elastic_charts/category_chart.js

snide · 2019-08-20T06:15:47Z

src-docs/src/views/elastic_charts/category_chart.js

+      <Fragment>
+        <EuiTitle size="xxs">
+          <h3>
+            Number of GitHub issues per visualization type


Looks like the legend has a small bug. It's reporting the hover values rather than an aggregation?

There is no "aggregation" value on the legend to be applied.
We for sure have to remove the value when not on hover (it's used on tsvb to show the last bucket but for time series, for ordinal values is not useful).

thompsongl · 2019-08-20T20:56:04Z

PR adding the chart themes to dist: cchaos#23

src-docs/src/views/elastic_charts/theming_example.js

src-docs/src/views/elastic_charts/category_example.js

src-docs/src/views/elastic_charts/category_chart.js

src-docs/src/views/elastic_charts/sizes.js

src-docs/src/views/elastic_charts/sparklines_example.js

gchaps · 2019-08-21T15:58:58Z

src-docs/src/routes.js

    ].map(example => createExample(example)),
  },
+  {
+    name: 'Elastic-Charts',


Is it ok to use Elastic Charts (no hyphen) in the TOC? Also, suggest this organization:

Elastic Charts

Creating charts

Theming via EUI

Coloring charts

Sizing charts

Time series charts

Categorical charts

Sparklines

I kept Sparklines with sizing but moved it to the second item just below Creating charts

src-docs/src/views/elastic_charts/theming_example.js

ryankeairns · 2019-08-21T18:01:38Z

Just stopping by to say that I pulled this down and it is awesome. Really great work @cchaos !

Started creating EUI specific light theme

cchaos · 2019-08-23T17:09:41Z

Thanks @markov00, for reference, I've addressed most of your bullet points.

the copy/paste code will not work 100% on TS because of some missing types. From a JS prospective seems fine except the missing imports of all the components and the formatters

I converted the theme to TS, not too worried about the C/P code at the moment for TS users. Hopefully they'll get their own warnings in order to fix that and the imports.

each <Chart size={[undefined, 200]}> and similar props can be rewritten in a more “nice” way as size={{height:200}}

Fixed

it’s better to use enums for the following props: xScaleType, yScaleType, on every Position (left, right, top, bottom), AnnotationDomainType and TooltipType

I only found one instance that wasn't using an enum which was for a xScaleType prop. Do you see more?

we will change in some future version but all the ids needs to be created with getSpecId, getAxisId, getAnnotationId on TS world

I saw in your Storybook that you're just using straight strings for id's. You may want to then look into your documentation around that.

on the copied code the data is always written as data={TIME_DATA=[[0,1],[1,2]]} is that intentional?

Fixed

I think it's missing the histogram chart, that is best suited for all our time series in kibana as we are, most of the time, showing buckets of aggregated values...

I didn't realize there was a particular chart type for that use case. I'll agree it would be good to have a documentation example solely for that. I think it's out of scope right now, but created a github issue to follow up on. #2254

cchaos · 2019-08-23T17:50:15Z

Jenkins, test this

thompsongl · 2019-08-23T21:50:37Z

Looking back, I don't think a single CI run has succeeded. Before the lint errors were resolved, CI was failing before it even got to eslint.

14:08:17 error @elastic/charts@10.1.1: The engine "node" is incompatible with this module. Expected version "10.15.2". Got "8.16.1"
14:08:17 error Found incompatible module

Removes the need for `mergeWithDefaultTheme`

thompsongl · 2019-08-23T21:57:07Z

@chandlerprall Does the CI include devDeps when it installs?

snide

🚀

chandlerprall · 2019-09-03T16:59:19Z

@cchaos pushed fix for the duplicate react versions, however elastic-charts is entering an infinite loop in IE11, both in these docs and in their published storyboard at elastic.github.com/elastic-charts , looks like the loop is caused by the resize observer /cc @markov00 @nickofthyme

nickofthyme · 2019-09-03T17:13:59Z

Thanks @chandlerprall I'll take a look

cchaos · 2019-09-03T17:40:05Z

Thanks @chandlerprall !

Has been addressed

…/CI to v10

chandlerprall · 2019-09-05T19:01:12Z

@cchaos I've updated the @elastic/elastic-charts dependency to latest version and made a change that I expect will let CI run against this branch.

cchaos · 2019-09-05T19:44:08Z

Thank you to all who made this possible 🥇

snide reviewed Aug 12, 2019

View reviewed changes

cchaos marked this pull request as ready for review August 14, 2019 14:26

cchaos requested a review from markov00 August 14, 2019 14:27

cchaos requested review from gchaps, snide and thompsongl August 19, 2019 19:39

cchaos force-pushed the docs-elastic-charts branch from 138782e to a2a9269 Compare August 19, 2019 21:45

snide reviewed Aug 20, 2019

View reviewed changes

gchaps reviewed Aug 20, 2019

View reviewed changes

gchaps reviewed Aug 21, 2019

View reviewed changes

cchaos force-pushed the docs-elastic-charts branch from d00792c to 0aa4b73 Compare August 21, 2019 23:00

markov00 and others added 8 commits August 22, 2019 19:33

Add basic example of elast-chart use.

8740c43

Added bar, line, area simple examples

e2b38f4

Started creating EUI specific light theme

Fix webpack

4125b93

Update charts

53494cf

Rename and add another doc folder

cbe53c0

Started the extras page

9e3e88a

Update to 4.0.0

72182d2

Trying to get Settings to know current theme

49452b0

cchaos added 4 commits August 23, 2019 12:54

update compiler from js to ts

5ac2c6a

Update chart sizing to object style

6e69b68

enum for xScaleType

a9e70f1

Remove TIME_DATA= from copied code

520aadc

cchaos mentioned this pull request Aug 23, 2019

[Charts] Add guidelines section for HistogramBarSeries chart #2254

Closed

cchaos requested a review from thompsongl August 23, 2019 17:20

cchaos added 2 commits August 23, 2019 14:41

target also has to be ts

b68c56c

Renaming exports to use Charts in the name

c259f04

Upgrade to Charts v10.2.0

cb7a843

Removes the need for `mergeWithDefaultTheme`

snide approved these changes Aug 23, 2019

View reviewed changes

thompsongl mentioned this pull request Aug 29, 2019

Update to node 10 #2274

Closed

Unify react, react-dom, and react-is versions

ecf8430

Merge branch 'master' into docs-elastic-charts

5eb1d34

Update elastic-charts to latest version, update node version in nvmrc…

621b92c

…/CI to v10

cchaos and others added 2 commits September 5, 2019 15:04

Merge branch 'master' into docs-elastic-charts

53f8956

change docker image and build step to use node 10

24b4f3c

cchaos merged commit d4a2fb6 into elastic:master Sep 5, 2019

thompsongl mentioned this pull request Sep 6, 2019

Upgrade EUI to 13.8.1 elastic/kibana#45052

Merged

thompsongl pushed a commit to thompsongl/eui that referenced this pull request Sep 10, 2019

Adding docs for elastic-charts (elastic#2209)

bf5f997

cchaos deleted the docs-elastic-charts branch October 22, 2019 20:03

Conversation

cchaos commented Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. Using elastic-charts and EUI together

2. Time series charts

3. Categorical charts

4. Sizing/Sparklines

Checklist

Uh oh!

markov00 commented Aug 8, 2019

Uh oh!

cchaos commented Aug 12, 2019

Uh oh!

snide left a comment • 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!

markov00 commented Aug 13, 2019

Uh oh!

cchaos commented Aug 14, 2019

Uh oh!

cchaos commented Aug 14, 2019

Uh oh!

cchaos commented Aug 19, 2019

Uh oh!

snide commented Aug 19, 2019

Uh oh!

thompsongl commented Aug 19, 2019

Uh oh!

snide left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

snide Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

markov00 Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

thompsongl commented Aug 20, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gchaps Aug 21, 2019

Choose a reason for hiding this comment

Uh oh!

cchaos Aug 22, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryankeairns commented Aug 21, 2019

Uh oh!

cchaos commented Aug 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cchaos commented Aug 23, 2019

Uh oh!

thompsongl commented Aug 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

cchaos commented Aug 8, 2019 •

edited

Loading

snide left a comment •

edited

Loading

cchaos commented Aug 23, 2019 •

edited

Loading

thompsongl commented Aug 23, 2019 •

edited

Loading

chandlerprall commented Sep 5, 2019 •

edited

Loading