Skip to content
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

Merged
merged 10 commits into from
Aug 21, 2019
Merged

[Canvas] Canvas embeddable #39839

merged 10 commits into from
Aug 21, 2019

Conversation

crob611
Copy link
Contributor

@crob611 crob611 commented Jun 27, 2019

Summary

Aug-01-2019 22-42-01

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 and savedVisualization. All of these will accept a filter context, so changes to the filters in the workpad should immediately be reflected in the embedded object.

Aug-01-2019 22-48-35

There will be a few followups to this.

  1. Making an argument type component that will show in the right hand sidebar for changing the savedObject that is powering the embeddable.
  2. The embeddables on the page reload as you go from page to page.

This is going to be failing CI until #42502 is merged

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@crob611 crob611 force-pushed the canvas-embeddable branch 2 times, most recently from 46aaa90 to 6a67cc7 Compare July 22, 2019 14:05
@elastic elastic deleted a comment from elasticmachine Jul 26, 2019
@elastic elastic deleted a comment from elasticmachine Jul 26, 2019
@elastic elastic deleted a comment from elasticmachine Jul 26, 2019
@elastic elastic deleted a comment from elasticmachine Jul 26, 2019
@elastic elastic deleted a comment from elasticmachine Jul 26, 2019
@crob611 crob611 force-pushed the canvas-embeddable branch 2 times, most recently from aa4cd72 to b024db0 Compare July 30, 2019 12:16
@KOTungseth
Copy link
Contributor

For consistency, I'm wondering if we could rename the Embed object link to Add object, similar to the Add Element button?

@elastic elastic deleted a comment from elasticmachine Aug 2, 2019
@elastic elastic deleted a comment from elasticmachine Aug 2, 2019
@elastic elastic deleted a comment from elasticmachine Aug 2, 2019
@elastic elastic deleted a comment from elasticmachine Aug 2, 2019
@elastic elastic deleted a comment from elasticmachine Aug 2, 2019
@elastic elastic deleted a comment from elasticmachine Aug 2, 2019
@crob611 crob611 marked this pull request as ready for review August 2, 2019 02:54
@crob611 crob611 requested a review from a team as a code owner August 2, 2019 02:55
@crob611 crob611 added loe:x-large Extra Large Level of Effort v7.4.0 v8.0.0 Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas labels Aug 2, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-canvas

@stacey-gammon
Copy link
Contributor

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 title customization is lost when switching from view mode to edit mode. I think you'll need to somehow detect changes to the embeddables input and modify the expression so it's saved with the workpad. This will probably also be the case with per panel time range - I don't see that action but I"m not sure if you just didn't rebase bc that was only merged very recently.

Would having an expression that's like embedObject type="visualize" id="123" instead of savedVisualization id="123" not work because the input parameters are different? It'd be cool if someone in canvas could try to embed any registered embeddable. I know we talked about only a subset making sense in Canvas, but if they are modifying the expression directly, then maybe that could be a workaround if someone really wanted a third party embeddable plugin to load.

If you can figure out how to pass viewMode: view to the embeddable input when you switch out of edit mode, then you wont' get those dotted lines, you won't get the move cursor, and you'll also only see actions that are not editable actions.

import {
SavedObjectFinder,
SavedObjectMetaData,
} from 'ui/saved_objects/components/saved_object_finder';
Copy link
Contributor

@streamich streamich Aug 16, 2019

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

@streamich streamich added the Feature:Embedding Embedding content via iFrame label Aug 16, 2019
@@ -0,0 +1,68 @@
/*
Copy link
Contributor

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@stacey-gammon
Copy link
Contributor

stacey-gammon commented Aug 16, 2019

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)

@stacey-gammon
Copy link
Contributor

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 timeRange parameter.

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 timeRange input parameter.

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@clintandrewhall clintandrewhall left a 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';
Copy link
Contributor

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 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@clintandrewhall
Copy link
Contributor

@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?

cc: @ryankeairns @shaunmcgough

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@markov00
Copy link
Member

markov00 commented Aug 21, 2019

@AlonaNadler

the existing visualization in kibana should have a transparent background, not sure if it is something that should be done in elastic charts @markov00 @nickofthyme or

all elastic-charts charts have a transparent background

@stacey-gammon
Copy link
Contributor

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?

@crob611 crob611 added v7.5.0 and removed v7.4.0 labels Sep 3, 2019
crob611 pushed a commit to crob611/kibana that referenced this pull request Sep 11, 2019
* 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/
crob611 pushed a commit that referenced this pull request Sep 11, 2019
* [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
@AdrianSanchezLopez
Copy link

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

@timductive
Copy link
Member

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.

@AdrianSanchezLopez
Copy link

Thank you very much for the answer, looking forward for both the Maps and visualizations Canvas embeddables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Embedding Embedding content via iFrame impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:x-large Extra Large Level of Effort release_note:enhancement review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.