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] Add to Library Action #75098

Merged
merged 9 commits into from
Aug 20, 2020

Conversation

ThomThomson
Copy link
Contributor

@ThomThomson ThomThomson commented Aug 14, 2020

Summary

Counterpart / Opposite of #74905

Adds an embeddable panel action to add by value embeddables to the library, turning them into by reference embeddables. This action should work with any embeddable that implements the ReferenceOrValueEmbeddable interface from #74302.

The action has an implementation in attribute_service that comes with a saved object save modal and title duplication detection.

Aug-14-2020 18-14-49

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. using the 'unlink book from library item' action menu item.

  4. Use the 'Add to Library' action, being sure to save the embeddable with a unique name.

Checklist

For maintainers

@ThomThomson ThomThomson added Feature:Dashboard Dashboard related features Feature:Embedding Embedding content via iFrame release_note:roadmap Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 v7.10.0 labels Aug 14, 2020
@ThomThomson ThomThomson requested a review from a team August 14, 2020 22:15
@ThomThomson ThomThomson requested a review from a team as a code owner August 14, 2020 22:15
@elasticmachine
Copy link
Contributor

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

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.

Tested in Chrome on Mac OS X on a book embeddable example. Works as expected 👍

@Dosant
Copy link
Contributor

Dosant commented Aug 19, 2020

Got following error:

  1. Add a book by value
  2. Add to library
  3. change title in a popup
  4. Press save

There is error in console:

add_to_library_action.tsx:90 Uncaught (in promise) undefined

Looks like original error is swallowed:

And it is:

TypeError: Cannot assign to read only property 'title' of object '#<Object>'
    at onSave (attribute_service.tsx:78)
    at async Object.onSaveConfirmed [as onSave] (show_saved_object_save_modal.tsx:37)
    at async SavedObjectSaveModal.saveSavedObject (saved_object_save_modal.tsx:87)

Which means that object that belongs to dashboard state container directly mutated

  1. To not bump into this error, we could create shallow copies of objects instead of mutating in place
  2. If possible, let's make sure that error is not swallowed

@ThomThomson
Copy link
Contributor Author

Thanks for finding this Anton! Seems like I was focusing on testing only embeddables that had previously been in the library, not brand new ones. I've forwarded the error and used a shallow copy as suggested.

@ThomThomson ThomThomson force-pushed the feature/addToLibraryAction branch from 4f8c41d to f171553 Compare August 19, 2020 16:01
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
dashboard 176 +1 175

page load bundle size

id value diff baseline
dashboard 702.2KB +7.4KB 694.8KB

History

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

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

LGTM, tested on a dashboard using book embeddable.

@ThomThomson ThomThomson merged commit 056038c into elastic:master Aug 20, 2020
ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Aug 20, 2020
* created an add to library action that turns 'by value' embeddables into 'by reference' embeddables
ThomThomson added a commit that referenced this pull request Aug 20, 2020
* created an add to library action that turns 'by value' embeddables into 'by reference' embeddables
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