Replace Discover chart with elastic-charts #43788

markov00 · 2019-08-22T16:57:52Z

Summary

Replace PR #36517

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

The replacement implies few small changes: the tooltip configured is a vertical cursor, always present. This simplify the interaction with smaller bars and also simplify the hover over the edges when the bucket doesn't fully cover the interval. In those cases It will shown only one tooltip instead of two different one that describe the situation:
before

now

notes
The discover functional tests that rely on checking the SVG bar heights have been skipped out as elastic-charts currently renders in canvas until this task is completed: elastic/elastic-charts#215
elastic/elastic-charts#310
elastic/elastic-charts#77

TODOs have been left so that these tests can be updated once elastic-charts provides a way to handle integration tests, and a meta issue will be created to track them

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

…e annotation

elasticmachine · 2019-08-22T16:58:25Z

Pinging @elastic/kibana-app

ryankeairns · 2019-08-23T13:04:34Z

@mdefazio Here is the PR for the chart atop Discover.

mdefazio · 2019-08-23T13:29:38Z

I had a moment and threw together a quick sketch of a possible solution to minimize the amount of space the charts take at the top of the page. User would then have the ability to expand to show the labels/axis, etc.

Removes the redundant date field above the chart
Increases the size of the result/hit count
The styling of the chart is for presentation only—not suggesting a different style than EUI charts theme.

timroes · 2019-08-26T10:45:24Z

@mdefazio I like that design. I am just not sure if we should do that in this PR, since I think we want to save the configuration of the chart with the discover view. Also we should ideally at the same time move the hits. So I would suggest moving the toggle changes and positioning of hits into a separate PR, after the conversion to elastic-charts.

cc @kertal

elasticmachine · 2019-08-27T12:45:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-03T18:24:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

ui functional tests were validating chart svg shapes which is not possible with canvas

elasticmachine · 2019-09-04T14:57:55Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spalger · 2019-09-04T16:16:08Z

src/legacy/core_plugins/kibana/public/discover/directives/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.',


"Part of...partial data" feels a little redundant.

Suggested change

'Part of this bucket may contain partial data. The selected time range does not fully cover it.',

'The selected time range does not include this entire bucket, it may contain partial data.',

See a396959

elasticmachine · 2019-09-04T16:16:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-10T02:44:02Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-10T17:28:06Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

kertal · 2019-09-10T17:45:41Z

🎉

* master: Replace Discover chart with elastic-charts (elastic#43788) [skip ci][Maps] Update search document section with new features (elastic#44819) Revert "Revert "[ci] compress jobs for CI stability" (elastic#44584)" add src/plugins to the list of plugin dirs to watch (elastic#45033)

…ation_behaviour * 'master' of github.com:elastic/kibana: (24 commits) [Console] SQL template with triple quote in completion (elastic#45248) [ML] Data Frames: Cards as links (elastic#45254) fix(code/frontend): should show updating instead of cloning when updating (elastic#45238) fix(code/frontend): fix document search result from (elastic#45236) disable another flaky suite (elastic#45323) (elastic#45330) disable flaky suite (elastic#45105) skip flaky suite (elastic#43069) skip flaky suite (elastic#45089) disable jest suite that has no enabled tests (elastic#44250) disable flaky test (elastic#45317) disable flaky test (elastic#45315) [DOCS] Creates developer folder (elastic#45280) [SIEM] Changes ML conditional links to use tabs, fixes a small bug with null filterQuery (elastic#45218) [skip-ci][Maps] Update search docs (elastic#45307) Revert "[skip ci][Maps] Update search document section with ne… (elastic#45301) Prep visualizations plugin for NP migration. (elastic#44839) Replace Discover chart with elastic-charts (elastic#43788) [skip ci][Maps] Update search document section with new features (elastic#44819) Revert "Revert "[ci] compress jobs for CI stability" (elastic#44584)" add src/plugins to the list of plugin dirs to watch (elastic#45033) ... # Conflicts: # src/legacy/core_plugins/console/public/src/utils.js # src/legacy/core_plugins/console/public/tests/src/utils_string_expanding.txt

liza-mae · 2019-09-18T17:14:28Z

Hi @markov00 this PR is labeled v7.5.0 but was not back ported, was it suppose to be or is it mislabeled?

markov00 · 2019-09-19T07:43:03Z

@liza-mae it needs to be back ported to 7.5, backporting soon

* use elastic-charts for histogram * add class accessibility * specify onElementClick type annotation * set chartElement tooltip type to Follow * use moment methods for now annotation logic * move historam inside directive folder * remove unused timechart directive * remove dependency from tsvb brush handler * remove non-required class to fix tooltip overflow * change the cursor/crosshair * fix(ie11): add fixed width for header text * fix: annotation colors on dark theme * unpdate click and brush ui functional tests * move functional tests to percy

Emma Cunningham and others added 6 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

[discover] set chartElement tooltip type to Follow

eb49a08

[discover] use moment methods for now annotation logic

ce5c42d

Merge branch 'master' into discover-chart-replace

ab1092d

markov00 requested a review from a team as a code owner August 22, 2019 16:57

markov00 added enhancement New value added to drive a business result Feature:Discover Discover Application Feature:ElasticCharts Issues related to the elastic-charts library release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Aug 22, 2019

markov00 mentioned this pull request Aug 22, 2019

[discover] use elastic-charts for histogram #36517

Closed

7 tasks

markov00 added the WIP Work in progress label Aug 22, 2019

markov00 changed the title ~~[discover] use elastic-charts for histogram~~ [wip] [discover] use elastic-charts for histogram Aug 22, 2019

markov00 added 4 commits August 22, 2019 19:02

refactor: move historam inside directive folder

b78fb2c

refactor: remove unused timechart directive

1f455c5

refactor: remove dependency from tsvb brush handler

5fc881f

refactor: remove unrequired class to fix tooltip overflow

aeaf295

markov00 added 3 commits August 26, 2019 16:42

WIP

d7d763b

Upgrade elastichart library and change the cursor/crosshair

6f9fc20

Merge branch 'master' into discover-chart-replace

0b41910

markov00 changed the title ~~[wip] [discover] use elastic-charts for histogram~~ Replace Discover chart with elastic-charts Aug 27, 2019

markov00 removed the WIP Work in progress label Aug 27, 2019

kertal self-requested a review August 27, 2019 12:10

nickofthyme added the v8.0.0 label Sep 4, 2019

nickofthyme added 2 commits September 4, 2019 08:41

move functional tests to percy

c65a7c3

ui functional tests were validating chart svg shapes which is not possible with canvas

wait for loading to complete for percy screenshots

72cf456

emmacunningham mentioned this pull request Sep 4, 2019

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

Closed

32 tasks

spalger reviewed Sep 4, 2019

View reviewed changes

nickofthyme added v7.4.0 v7.5.0 and removed v7.4.0 labels Sep 4, 2019

nickofthyme added 4 commits September 9, 2019 19:03

Merge branch 'master' into discover-chart-replace

ff9f75b

Merge branch 'master' into discover-chart-replace

f9bbb01

update partial bucket message

a396959

fix merge conflicts and functional test errors

6298388

update discover page with changes to broser util function

0141ddb

nickofthyme merged commit 5cd40b4 into elastic:master Sep 10, 2019

kertal mentioned this pull request Sep 13, 2019

Design adaptions of Discover's chart #45629

Closed

markov00 mentioned this pull request Sep 19, 2019

[7.x] Replace Discover chart with elastic-charts (#43788) #46115

Merged

nickofthyme mentioned this pull request Sep 27, 2019

Replace histogram on Discover by EUI chart component #17520

Closed

nickofthyme mentioned this pull request Oct 15, 2019

[meta] Chart Replacement Progress elastic/elastic-charts#417

Closed

22 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Discover chart with elastic-charts #43788

Replace Discover chart with elastic-charts #43788

markov00 commented Aug 22, 2019 •

edited

Loading

elasticmachine commented Aug 22, 2019

ryankeairns commented Aug 23, 2019

mdefazio commented Aug 23, 2019

timroes commented Aug 26, 2019

elasticmachine commented Aug 27, 2019

elasticmachine commented Sep 3, 2019

elasticmachine commented Sep 4, 2019

spalger Sep 4, 2019 •

edited

Loading

nickofthyme Sep 10, 2019

elasticmachine commented Sep 4, 2019

elasticmachine commented Sep 10, 2019

elasticmachine commented Sep 10, 2019

kertal commented Sep 10, 2019

liza-mae commented Sep 18, 2019

markov00 commented Sep 19, 2019

	'Part of this bucket may contain partial data. The selected time range does not fully cover it.',
	'The selected time range does not include this entire bucket, it may contain partial data.',

Replace Discover chart with elastic-charts #43788

Replace Discover chart with elastic-charts #43788

Conversation

markov00 commented Aug 22, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Aug 22, 2019

ryankeairns commented Aug 23, 2019

mdefazio commented Aug 23, 2019

timroes commented Aug 26, 2019

elasticmachine commented Aug 27, 2019

💚 Build Succeeded

elasticmachine commented Sep 3, 2019

💚 Build Succeeded

elasticmachine commented Sep 4, 2019

💚 Build Succeeded

spalger Sep 4, 2019 • edited Loading

Choose a reason for hiding this comment

nickofthyme Sep 10, 2019

Choose a reason for hiding this comment

elasticmachine commented Sep 4, 2019

💚 Build Succeeded

elasticmachine commented Sep 10, 2019

💔 Build Failed

elasticmachine commented Sep 10, 2019

💚 Build Succeeded

kertal commented Sep 10, 2019

liza-mae commented Sep 18, 2019

markov00 commented Sep 19, 2019

markov00 commented Aug 22, 2019 •

edited

Loading

spalger Sep 4, 2019 •

edited

Loading