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

More saved object embeddable examples #61749

Conversation

stacey-gammon
Copy link
Contributor

@stacey-gammon stacey-gammon commented Mar 29, 2020

Changes pulled from #61692:

  • Changed the way savedObjectIds work in embeddable containers - they are no longer special, they are just another form of input, and so will now be kept inside panels[x].explicitInput instead of on panels[x] directly.
  • Add panel flyout now grabs the type of saved object from the embeddable factory rather than coupling embeddable factory type to saved object type.
  • Add panel flyout calls container.addNewEmbeddable rather than container.addSavedObjectEmbeddable

TODO:

  • remove the special factory.createFromSavedObject function. Embeddable factories are meant to be nothing more than registered createEmbeddable functions. Input into factory is the input into the Embeddable.

New examples:

  • TodoSoEmbeddable shows how a single embeddable type can be used both for "by value" and "by reference". There is a save action which mimics some of the flow we aim to have for "Time to dashboard" project. The idea is:
    • Input contains an optional savedObjectId, as well as optional saved object attributes. You can think of these as editable fields of the saved object. If the savedObjectId is undefined, there must be enough data in input to still correctly render the Embeddable. It can be:
  input: {
    savedObjectId: 123,
    attributes: {
      task: "Edited task",
      title: "Edited Title",
    }
  }
 
output: {
  savedAttributes: {
    task: "Persisted task",
    title: "Persisted title",
  }
}

This allows the user to:

  • use a "by value" representation
  • convert a "by value" representation to a saved object
  • Edit a "by reference" representation, and optionally save those edits to the saved object, or just keep them as overrides inside the container.

Screen Shot 2020-03-29 at 5 01 53 PM

Screen Shot 2020-03-29 at 5 01 42 PM

The one caveat I discovered with this approach is how containers work. The container implementation works off input.panels. If we have a "by reference" dashboard, we don't want to also force the use to set input.panels, you should be able to simply instantiate with:

  input: {
    savedObjectId: 123,
  }
 
output: {
  panels: PanelState,
}

I think this points to a code smell on the Containers that they should be an interface, not a base class, and for some of the complicated logic, we should just offer as helper functions. While it's not ideal, we should be able to move forward with this until the container class can be refactored. Alternatively we could probably go with a separate wrapped embeddable approach for containers, as explored in #61692.

Related:

@botelastic botelastic bot added Feature:Embedding Embedding content via iFrame Team:AppArch labels Mar 29, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@stacey-gammon stacey-gammon force-pushed the 2020-03-10-more-embed-saved_object_examples branch 2 times, most recently from 35e4e40 to 436b58f Compare March 29, 2020 21:02
@ryankeairns
Copy link
Contributor

@stacey-gammon In the Time to Dashboard project we've referred to the action as 'Add to library', which raises a question for me:

Are we creating a new object when we add to the library or is it the same object but now it is 'flagged' as being visible in the Viz library?

@stacey-gammon
Copy link
Contributor Author

Are we creating a new object when we add to the library or is it the same object but now it is 'flagged' as being visible in the Viz library?

If the embeddable is not already backed by a saved object, you'd be creating a new one when you click "Save to library". If you try to edit an embeddable that is already linked to a saved object, I had the button say "update library item". We can change that though. We could have an "unlink" functionality (just clears the saved object from the input but keeps the attributes).

@stacey-gammon stacey-gammon force-pushed the 2020-03-10-more-embed-saved_object_examples branch 10 times, most recently from 01031e1 to f4ceac3 Compare March 31, 2020 15:40
Add embeddable via saved object example
@stacey-gammon stacey-gammon force-pushed the 2020-03-10-more-embed-saved_object_examples branch from f4ceac3 to fcd2ccf Compare March 31, 2020 17:22
Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

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

This is very big to review. I like the concept of having a saved object embeddable separately.

@stacey-gammon
Copy link
Contributor Author

Yea, it's a big review, it is built off of this smaller PR though: #61692

However that one hasn't any reviews yet so I may just close it and ask reviews for this one.

Regarding your comment, you prefer the approach where there are two embeddable factories registered, one for "by value" one for "by ref"? This is the approach I took in the sample in #61692 but thought it actually might be easier for devs to only have to register one type of factory, hence this PR removes the example in that earlier PR and adds another example.

Note that the TodoSoEmbeddable actually encapsulates everything TodoEmbeddable does but also can be saved as a saved object. Meaning you only need to use TodoSoEmbeddable to get both by value and by reference functionality. I keep TodoEmbeddable around only as a simpler example to show some of the other concepts.

@stacey-gammon stacey-gammon marked this pull request as ready for review March 31, 2020 19:29
@stacey-gammon stacey-gammon requested a review from a team March 31, 2020 19:29
@stacey-gammon stacey-gammon requested a review from a team as a code owner March 31, 2020 19:29
@stacey-gammon stacey-gammon added v7.8.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Mar 31, 2020
@majagrubic
Copy link
Contributor

Sorry, I didn't look at the PR carefully, and thought that two factories was the approach you were taking, as in the prototype. I thought that adding an optional savedObjectId makes things a bit messy. But on a second thought, creating the right embeddable based on certain parameter is according to a definition of factory, so this is actually better.

I managed to create a new embeddable, both backed by a saved object and without it - works great 🎉
One nit: do you happen to know why the saved object embeddable is not showing up in the management console? I assume it has something to do with the example run?

@stacey-gammon
Copy link
Contributor Author

One nit: do you happen to know why the saved object embeddable is not showing up in the management console? I assume it has something to do with the example run?

I believe saved objects types have to explicitly opt in to showing up in management. Management doesn't just show every new saved object type automatically.

@stacey-gammon
Copy link
Contributor Author

Note: would like to merge #61165 first since I'll have to resolve some conflicts afterward.

@kibanamachine
Copy link
Contributor

💔 Build Failed

Failed CI Steps

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@stacey-gammon
Copy link
Contributor Author

I'm going to pull a bit of functionality out of this PR to make it smaller. Stay tuned.

@stacey-gammon
Copy link
Contributor Author

I'm going to get these two PRs merged first:

  • Embeddable clean up #62486
  • Add embeddable via saved object example #61692
    • I think this has a decent example to start with since it's simpler. If all you want is an embeddable that is backed by a saved object, this is an easier example to follow. In terms of complexity, the next step will be following up on this example, which shows how a single embeddable that can switch from "by value" to "by reference". All of these I think are fine patterns depending on what the author of the embeddable wishes to support.

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 release_note:skip Skip the PR/issue when compiling release notes v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants