-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Replace Discover chart with elastic-charts #43788
Conversation
Pinging @elastic/kibana-app |
@mdefazio Here is the PR for the chart atop Discover. |
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.
|
@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 |
💚 Build Succeeded |
💚 Build Succeeded |
ui functional tests were validating chart svg shapes which is not possible with canvas
💚 Build Succeeded |
|
||
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.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Part of...partial data" feels a little redundant.
'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.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See a396959
💚 Build Succeeded |
💔 Build Failed |
💚 Build Succeeded |
🎉 |
* 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
Hi @markov00 this PR is labeled v7.5.0 but was not back ported, was it suppose to be or is it mislabeled? |
@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
* 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
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
strikethroughsto remove checklist items you don't feel are applicable to this PR.[ ] 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 tutorialsFor maintainers
[ ] This includes a feature addition or change that requires a release note and was labeled appropriately