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

[Dashboard First] Unlink from Library Action With ReferenceOrValueEmbeddable #74905

Conversation

ThomThomson
Copy link
Contributor

@ThomThomson ThomThomson commented Aug 12, 2020

Summary

Counterpart / Opposite of #75098

Adds an embeddable panel action to unlink by reference embeddables from their library item, turning them into by value embeddables. This action should work with any embeddable that implements the ReferenceOrValueEmbeddable interface from #74302.
Aug-12-2020 17-09-13

How to test this?

This is just one part of a much larger change, so all changes in this PR are hidden behind a configuration value. In order for the 'by value' workflow to be made default, some architectural changes need to be completed including #67931, #71499, and #61663

  1. you'll need to set the allowByValueEmbeddables config value to true:

    allowByValueEmbeddables: schema.boolean({ defaultValue: false }),

  2. There are no production ready embeddables that implement the ReferenceOrValueEmbeddable yet so you will have to start the developer examples. yarn start --run-examples

  3. Open a dashboard, and use the add panels functionality to add a Book embeddable. You should be able to then unlink it from its library item.

Checklist

For maintainers

…can be treated as either by reference or by value
…tions to test the referenceOrValueEmbeddable interface.
…omson/kibana into feature/unlinkFromLibraryAction2
@ThomThomson ThomThomson added Feature:Dashboard Dashboard related features release_note:roadmap Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 v7.10.0 labels Aug 12, 2020
@ThomThomson ThomThomson force-pushed the feature/unlinkFromLibraryActionWithInterface branch from f8a672d to 37cca10 Compare August 13, 2020 14:35
@ThomThomson ThomThomson marked this pull request as ready for review August 13, 2020 16:31
@ThomThomson ThomThomson requested a review from a team August 13, 2020 16:31
@ThomThomson ThomThomson requested a review from a team as a code owner August 13, 2020 16:31
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@@ -103,7 +104,12 @@ export class BookEmbeddable extends Embeddable<BookEmbeddableInput, BookEmbeddab
};

getInputAsValueType = async (): Promise<BookByValueInput> => {
return this.attributeService.getInputAsValueType(this.input);
const input =
Copy link
Contributor

Choose a reason for hiding this comment

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

can we simplify this statement a bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized that getting the explicitInput of an embeddable will be a common enough task.
The reason this is needed is so that the 'unlink' or 'add' actions don't make inherited input into explicit input.

I added a helper to the attribute service in the add to library PR #75098.:

public getExplicitInputFromEmbeddable(embeddable: IEmbeddable): ValType | RefType {
return embeddable.getRoot() &&
(embeddable.getRoot() as Container).getInput().panels[embeddable.id].explicitInput
? ((embeddable.getRoot() as Container).getInput().panels[embeddable.id].explicitInput as
| ValType
| RefType)
: (embeddable.getInput() as ValType | RefType);
}

It can be simplified a little bit with optional chaining, and I will update it to reflect that over there.

@botelastic botelastic bot added the Feature:Embedding Embedding content via iFrame label Aug 14, 2020
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.

I tested this according to instructions and can confirm adding and removing from library works as expected. Love the unit test with different scenarios, which make it very easy to follow the expected flow.

Two questions:

  1. If the panel is "by value", why is it still showing in the list of panels when adding a new panel? It shouldn't live outside of the dashboard, right?

  2. What happens if we save two panels with the same title? Eg:

  • I have a panel, it's by value
  • I clone it -> clone is by value
  • I add an original to the library -> works as expected
  • I add a clone to the library -> this works
    We normally don't allow saving two visualizations with the same name. Is this just an example that works like this? Or will this be an expected flow from now on?

type: embeddable.type,
explicitInput: { ...newInput, id: uuid.v4() },
};
dashboard.replacePanel(panelToReplace, newPanel);
Copy link
Contributor

Choose a reason for hiding this comment

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

not related to this PR, but really shows how much we need an updatePanel rather than replacePanel functionality 😞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I absolutely agree, fingers crossed that this #74253 will handle it.

@ThomThomson
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

code LGTM

@ThomThomson
Copy link
Contributor Author

Answers to the above questions from @majagrubic

  1. If the panel is "by value", why is it still showing in the list of panels when adding a new panel?
    This action is an unlink from library action, it does not remove the saved object. After un-linking, the saved object still exists, and it will show up in the library, but any edits to the by value version that is now on the dashboard will not affect the original library item because the link has been broken. With this functionality the library can almost become a home for 'templates'

  2. What happens if we save two panels with the same title?
    This is a little bit confusing, because we're testing this with the book_embeddable. The Book Embeddable has an 'add book to library action'. which (in this PR) doesn't have duplicate detection. Additionally, the 'add book to library' action only works on books, and is supposed to be used in the example plugins. In the Add to Library PR, [Dashboard First] Add to Library Action #75098 the action does come with duplicate detection (via the attribute service)

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
dashboard 175 +1 174

page load bundle size

id value diff baseline
dashboard 694.6KB +6.9KB 687.7KB

History

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

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.

Looks and works great!

@ThomThomson ThomThomson merged commit 2af2b60 into elastic:master Aug 18, 2020
ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Aug 18, 2020
…eddable (elastic#74905)

* Added an unlink from library action which works with the ReferenceOrValue interface. Once
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 18, 2020
* master:
  Skip failing test in CI (elastic#75266)
  [Task Manager] time out work when it overruns in poller (elastic#74980)
  [Drilldowns] misc improvements & fixes (elastic#75276)
  Small README note on bumping memory for builds (elastic#75247)
  [Security Solution][Detections] Adds exception modal tests (elastic#74596)
  [Dashboard] Sample data link does not work (elastic#75262)
  [Dashboard First] Unlink from Library Action With ReferenceOrValueEmbeddable (elastic#74905)
  [Form lib] Fix issue where serializer on fields are called on every change (elastic#75166)
  convert processor labels to sentence case (elastic#75278)
  [Monaco] Refactor the way XJSON grammar checker gets registered (elastic#75160)
  Clarify no documents error message when filtering by is_training (elastic#75227)
  [Lens] Fix crash when two layers xychart  switches to pie (elastic#75267)
  [Observability Homepage] Fix console error because of side effect (elastic#75258)
  [Usage Collection] Add `legacy=true` option to the /api/stats request in the docs (elastic#75146)
  [ML] Functional tests - re-activate DFA test suites (elastic#75257)
  GS providers improvements (elastic#75174)
  [Visualize] First version of by-value visualize editor (elastic#72256)
ThomThomson added a commit that referenced this pull request Aug 18, 2020
…eddable (#74905) (#75305)

* Added an unlink from library action which works with the ReferenceOrValue interface. Once
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 19, 2020
…emove-header

* saved-objects/version-on-create: (59 commits)
  remove version when loading sample data
  omit version from SO import/export
  Skip failing test in CI (elastic#75266)
  [Task Manager] time out work when it overruns in poller (elastic#74980)
  [Drilldowns] misc improvements & fixes (elastic#75276)
  Small README note on bumping memory for builds (elastic#75247)
  [Security Solution][Detections] Adds exception modal tests (elastic#74596)
  Revert "Revert "added missing core docs""
  Revert "Revert "added version to saved object bulk creation""
  [Dashboard] Sample data link does not work (elastic#75262)
  [Dashboard First] Unlink from Library Action With ReferenceOrValueEmbeddable (elastic#74905)
  [Form lib] Fix issue where serializer on fields are called on every change (elastic#75166)
  convert processor labels to sentence case (elastic#75278)
  [Monaco] Refactor the way XJSON grammar checker gets registered (elastic#75160)
  Clarify no documents error message when filtering by is_training (elastic#75227)
  [Lens] Fix crash when two layers xychart  switches to pie (elastic#75267)
  [Observability Homepage] Fix console error because of side effect (elastic#75258)
  [Usage Collection] Add `legacy=true` option to the /api/stats request in the docs (elastic#75146)
  [ML] Functional tests - re-activate DFA test suites (elastic#75257)
  GS providers improvements (elastic#75174)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features Feature:Embedding Embedding content via iFrame Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants