-
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
Expression: Add render mode and use it for canvas interactivity #83559
Conversation
Pinging @elastic/kibana-app (Team:KibanaApp) |
Pinging @elastic/kibana-presentation (Team:Presentation) |
Pinging @elastic/kibana-app-arch (Team:AppArch) |
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.
code LGTM
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!
@@ -92,6 +93,9 @@ export class ExpressionRenderHandler { | |||
event: (data) => { | |||
this.eventsSubject.next(data); | |||
}, | |||
getMode: () => { |
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.
nit: I'd prefer consistency.
getMode: () => { | |
getRenderMode: () => { |
// Warning: (ae-forgotten-export) The symbol "RenderMode" needs to be exported by the entry point index.d.ts | ||
// | ||
// (undocumented) | ||
getMode: () => RenderMode; |
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.
getMode: () => RenderMode; | |
getRenderMode: () => RenderMode; |
@@ -11,6 +11,7 @@ export const defaultHandlers: RendererHandlers = { | |||
destroy: () => action('destroy'), | |||
getElementId: () => 'element-id', | |||
getFilter: () => 'filter', | |||
getMode: () => 'display', |
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.
getMode: () => 'display', | |
getRenderMode: () => 'display', |
@@ -23,6 +23,9 @@ export const createHandlers = (): RendererHandlers => ({ | |||
getFilter() { | |||
return ''; | |||
}, | |||
getMode() { |
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.
getMode() { | |
getRenderMode() { |
@@ -139,6 +139,7 @@ export const getPieRenderer = (dependencies: { | |||
chartsThemeService={dependencies.chartsThemeService} | |||
paletteService={dependencies.paletteService} | |||
onClickValue={onClickValue} | |||
renderMode={handlers.getMode()} |
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.
renderMode={handlers.getMode()} | |
renderMode={handlers.getRenderMode()} |
@@ -212,6 +216,7 @@ export const getXyChartRenderer = (dependencies: { | |||
histogramBarTarget={dependencies.histogramBarTarget} | |||
onClickValue={onClickValue} | |||
onSelectRange={onSelectRange} | |||
renderMode={handlers.getMode()} |
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.
renderMode={handlers.getMode()} | |
renderMode={handlers.getRenderMode()} |
Fair point @clintandrewhall , I changed it to "getRenderMode" |
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
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 in Chrome and Firefox
* master: (41 commits) [Maps] fix code-owners (elastic#84265) [@kbn/utils] Clean target before build (elastic#84253) [code coverage] collect for oss integration tests (elastic#83907) [APM] Use `asTransactionRate` consistently everywhere (elastic#84213) Attempt to fix incremental build error (elastic#84152) Unskip "Copy dashboards to space" (elastic#84115) Remove expressions.legacy from README (elastic#79681) Expression: Add render mode and use it for canvas interactivity (elastic#83559) [deb/rpm] Move systemd service to /usr/lib/systemd/system (elastic#83571) [Security Solution][Resolver] Allow a configurable entity_id field (elastic#81679) [ML] Space permision checks for job deletion (elastic#83871) [build] Provide ARM build of RE2 (elastic#84163) TSVB should use "histogram:maxBars" and "histogram:barTarget" settings for auto instead of a default 100 buckets (elastic#83628) [Workplace Search] Initial rendering of Org Sources (elastic#84164) update geckodriver to 0.28 (elastic#84085) Fix timelion vis escapes single quotes (elastic#84196) [Security Solution] Fix incorrect time for dns histogram (elastic#83532) [DX] Bump TS version to v4.1 (elastic#83397) [Security Solution] Add endpoint policy revision number (elastic#83982) [Fleet] Integration Policies List view (elastic#83634) ...
Fixes #77161
Fixes #65630
This PR adds a new option
renderMode
to expression loader and renderer params which is passed from the embedding (e.g. Lens embeddable class) to the expression renderer. There it can be used to change the rendering of the chart.This PR puts the new option into use as well by extending the Lens embeddable input in the same way and using the
noInteractivity
mode for Lens visualizations embedded in Canvas (because filters don't work there anyway).The only user facing change is no brush functionality and no pointer cursor on click on Lens charts embedded in Canvas