-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Dashboard] Fix Lens and TSVB chart tooltip positioning relative to global headers #94247
Conversation
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.
Tested and works as expected - the header height should be pulled from the eui variable, otherwise this looks good to me. Failing tests seem to be easily fixable
@@ -61,6 +61,8 @@ type XYSettingsProps = Pick< | |||
legendPosition: Position; | |||
}; | |||
|
|||
const KBN_HEADER_OFFSET = 98; |
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.
This constant should be available from
import euiLightVars from '@elastic/eui/dist/eui_theme_light.json';
KBN_HEADER_OFFSET = 2* euiLightVarseuiHeaderHeightCompensation
(same for Lens)
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.
Thanks @flash1293 I'll pull from that value.
Also is there an easy way to tell what app you are in or better yet if the header is visible? Other than a querySelector
. I'd like to only add this padding when the tooltip is visible. Like canvas for instance may use an xy chart on full screen where the padding is not necessary.
Screen.Recording.2021-03-10.at.07.50.29.AM.mp4
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.
That's a good point - core.chrome.getIsVisible$
should give you an observable with this information. The biggest challenge is to pass it through to the right places :)
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.
That worked perfectly! Thanks! 🙏
xy charts in canvas
Screen.Recording.2021-03-10.at.09.04.54.AM.mp4
Lens charts in dashboard
Screen.Recording.2021-03-10.at.09.39.54.AM.mp4
Pinging @elastic/kibana-app (Team:KibanaApp) |
Since the body is the main scroll container in dashboard app, the Screen.Recording.2021-03-10.at.12.18.26.PM.mp4 |
@nickofthyme With the fixed viewport element to anchor things, do we even need the padding-top calculation and chrome/no-chrome detection logic in lens and vis_type_xy? It seems like it would be easier for the core code to just add a |
Great idea @flash1293, that also accounts for the I added the Screen.Recording.2021-03-11.at.11.37.33.AM.mp4 |
<div id="app-fixed-viewport" /> | ||
<div id="globalBannerList">{bannerComponent}</div> | ||
<AppContainer classes$={chrome.getApplicationClasses$()}>{appComponent}</AppContainer> |
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.
I understand the purpose, but I'm unsure why all this is necessary tbh. In 'stretch-to-height' mode apps, app-wrapper-panel
(or even .kibana-body
) already pretty much has the effective layout/positioning the added #app-fixed-viewport
element has, isn't it?
Couldn't you just use .app-wrapper-panel
or .kbnBody
as your boundary element?
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.
The .app-wrapper-panel
and the
#kibana-body` have a height of 100% of the entire dashboard and they scroll with the dashboard itself, using that as boundary doesn't block the tooltip from being blocked right below the headers.
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.
@markov00 is correct, I tried using every top-level element that was available including:
document.body
or.kbn-body
-- only at the top of the scroll height and moves when body is scrolled. See video below.#kibana-body
-- full scroll height goes beyond viewport as soon as you scroll.content
-- full scroll height goes beyond viewport as soon as you scroll.app-wrapper
-- full scroll height goes beyond viewport as soon as you scroll.app-wrapper-panel
-- full scroll height goes beyond viewport as soon as you scroll.application
-- full scroll height goes beyond viewport as soon as you scroll
You can see the height of these scroll elements on the video below being
~2500px
.
This was the last option unless someone else has any suggestions.
Screen.Recording.2021-03-11.at.11.49.31.AM.mp4
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.
The .app-wrapper-panel and the #kibana-body` have a height of 100% of the entire dashboard and they scroll with the dashboard itself
Oh, I thought it was for a stretch to height app. You're right, for dashboard, the body is taking the effective height of the dashboard.
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.
LGTM for core changes
@elasticmachine merge upstream |
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.
@flash1293 thanks for that, I think this could be somewhat of a solution for that elastic/elastic-charts#872 since scrolling tooltips would be a big change. But we are discussing the redesign of the tooltip now so I will add this as a case to consider. |
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.
The SCSS changes look fine to me. Approving.
@elasticmachine merge upstream |
💔 Build Failed
Failed CI Steps
Test FailuresKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/security/users·js.security app users should show the default rolesStandard Out
Stack Trace
Kibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/security/users·js.security app users should show the default rolesStandard Out
Stack Trace
Kibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/discover/reporting·ts.discover Discover CSV Export Generate CSV: new search generates a report from a new search with data: defaultStandard Out
Stack Trace
and 3 more failures, only showing the first 3. Metrics [docs]Async chunks
Page load bundle
Saved Objects .kibana field count
History
To update your PR or re-run it, just comment with: |
…-nav * 'master' of github.com:elastic/kibana: (106 commits) [Lens] don't use eui variables for zindex (elastic#96117) Remove /src/legacy (elastic#95510) skip flaky suite (elastic#95899) [Dashboard] Fix Lens and TSVB chart tooltip positioning relative to global headers (elastic#94247) fixes a skipped management x-pack test (elastic#96178) [App Search] API logs: Add log detail flyout (elastic#96162) [tech-debt] Remove defunct opacity parameters from EUI shadow functions (elastic#96191) Add Input Controls project configuration (elastic#96238) [file upload] document file upload privileges and provide actionable UI when failures occur (elastic#95883) Revert "TS Incremental build exclude test files (elastic#95610)" (elastic#96223) [App Search] Added Sample Response section to Result Settings (elastic#95971) [Maps] Safe-erase text-field (elastic#94873) [RAC][Alert Triage][TGrid] Update the Alerts Table (TGrid) API to implement `renderCellValue` (elastic#96098) [Maps] Enable all zoom levels for all users (elastic#96093) Use plugin version in its publicPath (elastic#95945) [Enterprise Search] Expose core.chrome.setIsVisible for use in Workplace Search (elastic#95984) [Workplace Search] Add sub nav and fix rendering bugs in Personal dashboard (elastic#96100) [OBS]home page is showing incorrect value of APM throughput (tpm) (elastic#95991) [Observability] Exploratory View initial skeleton (elastic#94426) [KQL] Fixed styles of KQL textarea for the K8 theme (elastic#96190) ... # Conflicts: # x-pack/plugins/snapshot_restore/__jest__/client_integration/helpers/restore_snapshot.helpers.ts
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Summary
Fixes #93495
Prevents the tooltip on xy charts and Lens from rendering outside of the
document.body
plus a top padding of98px
for the global header. This only occurs for long tooltips caused by many split and y accessors.Notice is all the screen recordings below, the tooltip is never positioned above the global header nor below the
window
, unless space is not permitted in either case.Added top-level element
This PR adds a new top-level element
#app-fixed-viewport
that is otherwise invisible and non-interactable to/by the user. This is to set the confines of which the chart tooltip can be positioned, accounting for the top headers in and out of fullscreen mode. This is required asbody
is the main scrolling element and will constantly shift up as you scroll and the.app-wrapper
element is the fullscrollHeight
of the content.Screen.Recording.2021-03-11.at.11.49.31.AM.mp4
Lens in editor
Screen.Recording.2021-03-10.at.07.34.05.AM.mp4
Lens in dashboard
Screen.Recording.2021-03-10.at.07.35.47.AM.mp4
xy chart in editor
Screen.Recording.2021-03-09.at.05.46.45.PM.mp4
xy chart in dashboard
Screen.Recording.2021-03-09.at.05.48.37.PM.mp4
Checklist