[discover] use elastic-charts for histogram by emmacunningham · Pull Request #36517 · elastic/kibana

emmacunningham · 2019-05-12T17:34:46Z

Summary

This PR swaps out the current implementation of the histogram chart in Discover to use elastic-charts instead.

The discover functional tests that rely on checking the SVG bar heights have been commented out as elastic-charts currently renders in canvas; TODOs have been left so that these tests can be updated once elastic-charts provides a way to handle integration tests.

Checklist

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

This was checked for cross-browser compatibility, including a check against IE11
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

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2019-05-12T18:53:38Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-21T17:11:52Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-21T22:08:05Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-22T22:55:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-13T15:19:00Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-14T23:28:32Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-17T21:55:57Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-18T00:50:32Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-19T17:36:25Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-01T08:36:01Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-01T09:48:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-03T07:25:24Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-08T18:06:06Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-16T21:25:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-18T17:17:50Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-24T23:44:36Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-29T20:01:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

kertal

It's great to have this replaced, I've checked it out to test it in Chrome, and I wonder if this is intended, you don't need to hover a bar for the popover to display:

nickofthyme

Tested Locally, LGTM. Just a few comments and questions.

src/legacy/core_plugins/kibana/public/discover/components/histogram/histogram.tsx

…e annotation

emmacunningham · 2019-07-31T21:22:50Z

It's great to have this replaced, I've checked it out to test it in Chrome, and I wonder if this is intended, you don't need to hover a bar for the popover to display:

@kertal Ah, good catch! By default, the elastic-charts tooltip type is set to VerticalCursor which will snap to the nearest data point, which is why you're seeing this. I think for the case of the Discover histogram, it probably makes more sense for the tooltip type to be Follow, which has been updated in the latest commit. Now the tooltip is only visible when directly hovering over a bar chart element.

elasticmachine · 2019-07-31T22:24:13Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-31T23:24:15Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

kertal · 2019-08-02T12:30:27Z

@kertal Ah, good catch! By default, the elastic-charts tooltip type is set to VerticalCursor which will snap to the nearest data point, which is why you're seeing this. I think for the case of the Discover histogram, it probably makes more sense for the tooltip type to be Follow, which has been updated in the latest commit. Now the tooltip is only visible when directly hovering over a bar chart element.

@emmacunningham
thanks, works now, Code LGTM, tested in Chrome, Firefox, Safari, IE11. All was fine only the following settings + rendering had some problems, especially the popup decided to stay outside:

kertal · 2019-08-02T12:42:49Z

We should open a ticket for adding/adapting functional testing to the new histogram. I'm linking a failed test case @spalger disabled recently, and as far as I can see, it's one of the test cases that need to be rewritten to work with the new histogram

#38361

markov00

I've done few tests locally and there are some issues that we need to solve before merging:

the tooltip is not shown on top of the other elements. I think it's a matter of the positioning of the chart container

- I'm not sure if that was also an issue with the previous chart, but if you choose a daily interval the x axis shows two times the same labels (maybe we should check the formatter)

- the tooltip can sometime overflow the chart container, involuntarily resizing the chart with a subsequent visualization of scrollbars

I know we switched to the follow cursor, but reintroduce the problem that we solve with the crosshair: hovering on small elements. On the following gift there is also 2 other issues:
- the text differs if you are hover that area on over the bar
- also the icon differs (one is the triangle with the i and the other is a circle with the i)
- we have also two different font weight (bold on the tooltip header, normal on the area tooltip)
Seems that the performance of the mouse over are degraded here in respect of the standard one of the elastic-charts. I can see that the tooltip sometimes lags a bit.
on chart playground

on discover chart

From the performance panel seems that there is some forced reflow style calculation(from 150ms to 300ms) that blocks the rendering

markov00 · 2019-08-06T16:38:01Z

test/functional/apps/discover/_discover.js

-        ];
-        await verifyChartData(expectedBarChartData);
-      });
+      // TODO: update this test when elastic-charts can support integration tests


Can you create a Kibana meta issue so we can track these skipped tests and we can reenable that later?

src/legacy/core_plugins/kibana/public/discover/components/histogram/directive.js

markov00 · 2019-08-06T17:21:23Z

src/legacy/core_plugins/kibana/public/discover/components/histogram/histogram.tsx

+    const partialDataText = i18n.translate('kbn.discover.histogram.partialData.bucketTooltipText', {
+      defaultMessage:
+        'Part of this bucket may contain partial data. The selected time range does not fully cover it.',
+    });


I think you can move out this translation, to avoid having to retranslate the same text every time you show that message

markov00 · 2019-08-06T17:26:47Z

src/legacy/core_plugins/kibana/public/discover/components/histogram/histogram.tsx

+    /**
+     * Deprecation: [interval] on [date_histogram] is deprecated, use [fixed_interval] or [calendar_interval].
+     * see https://github.com/elastic/kibana/issues/27410
+     * TODO: Once the Discover query has been update, we should change the below to use the new field


can you create a meta issue for that?

markov00 · 2019-08-06T17:30:12Z

src/legacy/core_plugins/kibana/public/discover/controllers/discover.js

can we create our own version of the brushHandler? In the next future we can add a set of kibana utilities for elastic-charts to add that

This PR remove the unnecessary style recalculation caused by chancing the cursor style on the document body from the chart_state. This was noticeable on this PR elastic/kibana#36517 (review) where the style reflow calculation tooks up to 75ms. This PR decouple the chart container from the chart renderer, allowing to set correctly the cursor changing the class without re-rendering the chart component, as the actual implementation. The cursor style now depends also on the status of the highlighted geometries. If one geometry is highlighted but no onElementClick or onElementOver listener is available, the cursor will be default if the brush is disabled, or crosshair if the brush is Enabled. The pointer is shown only if we have one between onElementClick or onElementOver enabled and we are over a geometry

# [10.0.0](v9.2.1...v10.0.0) (2019-08-21) ### Bug Fixes * **tooltip:** ie11 flex sizing ([#334](#334)) ([abaa472](abaa472)), closes [#332](#332) * decuple brush cursor from chart rendering ([#331](#331)) ([789f85a](789f85a)), closes [elastic/kibana#36517](elastic/kibana#36517) * remove clippings from chart geometries ([#320](#320)) ([ed6d0e5](ed6d0e5)), closes [#20](#20) ### Features * auto legend resize ([#316](#316)) ([659d27e](659d27e)), closes [#268](#268) * customize number of axis ticks ([#319](#319)) ([2b838d7](2b838d7)) * **theme:** base theme prop ([#333](#333)) ([a9ff5e1](a9ff5e1)), closes [#292](#292) ### BREAKING CHANGES * **theme:** remove `baseThemeType` prop on `Settings` component and `BaseThemeTypes` type. * `theme.legend.spacingBuffer` added to `Theme` type. Controls the width buffer between the legend label and value.

markov00 · 2019-08-22T16:59:46Z

closing this in favour of: #43788

This PR remove the unnecessary style recalculation caused by chancing the cursor style on the document body from the chart_state. This was noticeable on this PR elastic/kibana#36517 (review) where the style reflow calculation tooks up to 75ms. This PR decouple the chart container from the chart renderer, allowing to set correctly the cursor changing the class without re-rendering the chart component, as the actual implementation. The cursor style now depends also on the status of the highlighted geometries. If one geometry is highlighted but no onElementClick or onElementOver listener is available, the cursor will be default if the brush is disabled, or crosshair if the brush is Enabled. The pointer is shown only if we have one between onElementClick or onElementOver enabled and we are over a geometry

emmacunningham force-pushed the feat/discover-elastic-charts branch from 8523aed to ef8310b Compare May 21, 2019 20:56

emmacunningham force-pushed the feat/discover-elastic-charts branch from ef8310b to bd26bcc Compare May 22, 2019 21:19

emmacunningham force-pushed the feat/discover-elastic-charts branch 2 times, most recently from 035a696 to 1372ed3 Compare June 13, 2019 14:53

emmacunningham mentioned this pull request Jun 13, 2019

Use elastic-charts in Kibana Discover elastic/elastic-charts#191

Closed

32 tasks

emmacunningham added the Feature:ElasticCharts Issues related to the elastic-charts library label Jun 14, 2019

emmacunningham force-pushed the feat/discover-elastic-charts branch from 1372ed3 to de8f4a3 Compare June 14, 2019 22:55

emmacunningham force-pushed the feat/discover-elastic-charts branch from de8f4a3 to 6fc31ca Compare June 17, 2019 21:27

emmacunningham force-pushed the feat/discover-elastic-charts branch from 6fc31ca to 02a85a2 Compare June 17, 2019 23:54

emmacunningham force-pushed the feat/discover-elastic-charts branch from 02a85a2 to 1de8741 Compare June 19, 2019 16:34

emmacunningham force-pushed the feat/discover-elastic-charts branch from 1de8741 to e6db1d0 Compare July 1, 2019 08:04

emmacunningham force-pushed the feat/discover-elastic-charts branch from e6db1d0 to ea4546f Compare July 1, 2019 08:52

emmacunningham force-pushed the feat/discover-elastic-charts branch from ea4546f to a97a6f3 Compare July 3, 2019 06:25

emmacunningham force-pushed the feat/discover-elastic-charts branch from a97a6f3 to 13cf1d5 Compare July 8, 2019 17:07

emmacunningham force-pushed the feat/discover-elastic-charts branch from 13cf1d5 to 055340f Compare July 16, 2019 20:15

emmacunningham requested review from a team and markov00 July 25, 2019 00:11

emmacunningham added the release_note:skip Skip the PR/issue when compiling release notes label Jul 25, 2019

emmacunningham force-pushed the feat/discover-elastic-charts branch from 4e3b2a5 to bd47e6c Compare July 29, 2019 18:51

kertal reviewed Jul 31, 2019

View reviewed changes

nickofthyme approved these changes Jul 31, 2019

View reviewed changes

Emma Cunningham added 3 commits July 31, 2019 13:22

[discover] use elastic-charts for histogram

3048b58

[discover] add class accessibility

a3f5802

[discover] bump elastic-charts version and specify onElementClick typ…

b19e606

…e annotation

emmacunningham force-pushed the feat/discover-elastic-charts branch from bd47e6c to b19e606 Compare July 31, 2019 20:53

[discover] set chartElement tooltip type to Follow

eb49a08

[discover] use moment methods for now annotation logic

ce5c42d

kertal added the Feature:Discover Discover Application label Aug 2, 2019

kertal mentioned this pull request Aug 6, 2019

Selecting a period in the discover histogram does not result in a refresh. #42583

Closed

markov00 requested changes Aug 7, 2019

View reviewed changes

markov00 mentioned this pull request Aug 20, 2019

fix: decuple brush cursor from chart rendering elastic/elastic-charts#331

Merged

2 tasks

markov00 mentioned this pull request Aug 22, 2019

Replace Discover chart with elastic-charts #43788

Merged

4 tasks

markov00 closed this Aug 22, 2019

Conversation

emmacunningham commented May 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented May 12, 2019

💔 Build Failed

Uh oh!

elasticmachine commented May 21, 2019

💔 Build Failed

Uh oh!

elasticmachine commented May 21, 2019

💔 Build Failed

Uh oh!

elasticmachine commented May 22, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Jun 13, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jun 14, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jun 17, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jun 18, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Jun 19, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Jul 1, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jul 1, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Jul 3, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jul 8, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jul 16, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Jul 18, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jul 24, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Jul 29, 2019

💚 Build Succeeded

Uh oh!

kertal 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!

nickofthyme left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emmacunningham commented Jul 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jul 31, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Jul 31, 2019

💚 Build Succeeded

Uh oh!

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

Uh oh!

Uh oh!

emmacunningham commented May 12, 2019 •

edited

Loading

kertal left a comment •

edited

Loading

emmacunningham commented Jul 31, 2019 •

edited

Loading

kertal commented Aug 2, 2019 •

edited

Loading

kertal commented Aug 2, 2019 •

edited

Loading

markov00 left a comment •

edited

Loading