-
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
[Canvas] Canvas embeddable #39839
[Canvas] Canvas embeddable #39839
Conversation
ca780ef
to
4e24b00
Compare
46aaa90
to
6a67cc7
Compare
aa4cd72
to
b024db0
Compare
For consistency, I'm wondering if we could rename the |
e188ea7
to
740454a
Compare
Pinging @elastic/kibana-canvas |
This is so awesome to see @crob611!! Great job! Noticed a few things. Happy to help try to work through them with you. We may want to prioritize some other embeddable features along for this to work better. Filtering on a visualization doesn't work because of #43411 Filtering on a saved search throws a console error, but the UI should probably hide the magnifying glass. We should tackle #9220 to solve this for Canvas so you can just temporarily turn all that interaction off. The Would having an expression that's like If you can figure out how to pass |
import { | ||
SavedObjectFinder, | ||
SavedObjectMetaData, | ||
} from 'ui/saved_objects/components/saved_object_finder'; |
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.
FYI #43416 — these components are expected to live in src/plugins/kibana_react
in 7.5
@@ -0,0 +1,68 @@ | |||
/* |
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 name all these files with snake case to be consistent with the rest of the code base (looks like a mix in this folder).
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.
+1
Actually, scratch the error about filtering not working on saved searches, it looks like this is a bug on master. Guess we have a gap in our test coverage! (see #43453) |
So, there is some strangeness with per panel time ranges - one, because input changes happening from actions aren't preserved inside the expression, but also because time range is being passed down via filters and not the I didn't actually test this out when implementing the feature, so I'd be interested to see if the latter thing causes weirdness on dashboard too. I'm actually not sure what ends up trumping what, if you pass a time range filter and another time range via |
💔 Build Failed |
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'm going to approve to unblock you... but be sure to address @stacey-gammon feedback about file names and mine about non-relative imports before shipping.
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { ExpressionType } from '../../../../../../src/plugins/data/common/expressions'; |
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.
Did changing this to src/plugins/data/common/expressions
not work for you?
@@ -0,0 +1,68 @@ | |||
/* |
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.
+1
@stacey-gammon We chatted in today's review that this PR is getting large, but the functionality is there... we're going to merge this and track bugs (e.g. full-screen state, expression state, filters) in separate issues. It will make the code easier to review. I'm hoping to impose upon you a bit: can you help log issues so we can make sure the experience in Canvas is what we're expecting? |
💚 Build Succeeded |
all elastic-charts charts have a transparent background |
Sorry, just got home from a trip so haven't been able to respond or file any bugs. Would you feel comfortable releasing 7.4 with the bugs mentioned above? Are Embeddables in Canvas marked as Beta? |
* Adds embeddable objects to canvas * Handle embeddable_api -> NewPlatform changes * Addressing PR feedback * Properly mock new platform * Snake case filenames{ * Switch relative paths to src/
* [Canvas] Canvas embeddable (#39839) * Adds embeddable objects to canvas * Handle embeddable_api -> NewPlatform changes * Addressing PR feedback * Properly mock new platform * Snake case filenames{ * Switch relative paths to src/ * Fix import paths for TimeRange * Adds i18n components file
Is this feature supposed to already in Kibana v 7.5.0? I'm using that version at the moment but can't find this option anywhere. If is not yet released, any ETA or expected version? Thanks |
Hi @AdrianSanchezLopez, we have broken up the work here. Our goal is to release Maps as embeddables in 7.6 and other visualizations in 7.7. This isn't a guarantee, there is some rework to do with how Canvas filters elements but this is the rough goal. |
Thank you very much for the answer, looking forward for both the Maps and visualizations Canvas embeddables. |
Summary
This adds the ability to embed anything that uses the Kibana Embeddable API into a Canvas Workpad by adding a new "embeddable" type to the interpreter, as well as an embeddable renderer.
We want to control specifically which embeddables are allowed, so each allowed embeddable will have it's own interpreter function. This PR adds functions
savedSearch
,savedMap
andsavedVisualization
. All of these will accept a filter context, so changes to the filters in the workpad should immediately be reflected in the embedded object.There will be a few followups to this.
This is going to be failing CI until #42502 is merged
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers