Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[discover] use elastic-charts for histogram #36517

Conversation

emmacunningham
Copy link

@emmacunningham emmacunningham commented May 12, 2019

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.

For maintainers

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

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

💔 Build Failed

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

💚 Build Succeeded

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

💔 Build Failed

@emmacunningham emmacunningham added the Feature:ElasticCharts Issues related to the elastic-charts library label Jun 14, 2019
@emmacunningham emmacunningham force-pushed the feat/discover-elastic-charts branch from 1372ed3 to de8f4a3 Compare June 14, 2019 22:55
@elasticmachine
Copy link
Contributor

💔 Build Failed

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

💔 Build Failed

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

💚 Build Succeeded

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

💚 Build Succeeded

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

💔 Build Failed

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

💚 Build Succeeded

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

💔 Build Failed

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

💔 Build Failed

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

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@emmacunningham emmacunningham requested review from markov00 and a team July 25, 2019 00:11
@emmacunningham emmacunningham added the release_note:skip Skip the PR/issue when compiling release notes label Jul 25, 2019
@emmacunningham emmacunningham force-pushed the feat/discover-elastic-charts branch from 4e3b2a5 to bd47e6c Compare July 29, 2019 18:51
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
image

Copy link
Contributor

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

emmacunningham commented Jul 31, 2019

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:
image

@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
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@kertal
Copy link
Member

kertal commented Aug 2, 2019

@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:

Bildschirmfoto 2019-08-02 um 14 14 07

@kertal kertal added the Feature:Discover Discover Application label Aug 2, 2019
@kertal
Copy link
Member

kertal commented Aug 2, 2019

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

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Screenshot 2019-08-07 at 12 04 08

- 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)

Screenshot 2019-08-07 at 12 13 56

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

Aug-07-2019 12-24-01

  • 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)
      Aug-07-2019 12-26-00
  • 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
    Aug-07-2019 12-29-31
    on discover chart
    Aug-07-2019 12-29-50

From the performance panel seems that there is some forced reflow style calculation(from 150ms to 300ms) that blocks the rendering
Screenshot 2019-08-07 at 12 32 51

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.',
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/**
* 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you create a meta issue for that?

@@ -72,6 +72,7 @@ import { buildVislibDimensions } from 'ui/visualize/loader/pipeline_helpers/buil
import 'ui/capabilities/route_setup';

import { data } from 'plugins/data/setup';
import { brushHandler } from '../../../../metrics/public/lib/create_brush_handler';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

markov00 added a commit to elastic/elastic-charts that referenced this pull request Aug 21, 2019
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
markov00 pushed a commit to elastic/elastic-charts that referenced this pull request Aug 21, 2019
# [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
Copy link
Member

closing this in favour of: #43788

@markov00 markov00 closed this Aug 22, 2019
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
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
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
# [10.0.0](elastic/elastic-charts@v9.2.1...v10.0.0) (2019-08-21)

### Bug Fixes

* **tooltip:** ie11 flex sizing ([opensearch-project#334](elastic/elastic-charts#334)) ([2683333](elastic/elastic-charts@2683333)), closes [opensearch-project#332](elastic/elastic-charts#332)
* decuple brush cursor from chart rendering ([opensearch-project#331](elastic/elastic-charts#331)) ([b5b8dde](elastic/elastic-charts@b5b8dde)), closes [elastic/kibana#36517](elastic/kibana#36517)
* remove clippings from chart geometries ([opensearch-project#320](elastic/elastic-charts#320)) ([497efa4](elastic/elastic-charts@497efa4)), closes [opensearch-project#20](elastic/elastic-charts#20)

### Features

* auto legend resize ([opensearch-project#316](elastic/elastic-charts#316)) ([be4a53d](elastic/elastic-charts@be4a53d)), closes [opensearch-project#268](elastic/elastic-charts#268)
* customize number of axis ticks ([opensearch-project#319](elastic/elastic-charts#319)) ([a7a4ecd](elastic/elastic-charts@a7a4ecd))
* **theme:** base theme prop ([opensearch-project#333](elastic/elastic-charts#333)) ([0b38770](elastic/elastic-charts@0b38770)), closes [opensearch-project#292](elastic/elastic-charts#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Discover Discover Application Feature:ElasticCharts Issues related to the elastic-charts library release_note:skip Skip the PR/issue when compiling release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants