Skip to content

[Dashboard First] Add to Library Action#75098

Merged
ThomThomson merged 9 commits intoelastic:masterfrom
ThomThomson:feature/addToLibraryAction
Aug 20, 2020
Merged

[Dashboard First] Add to Library Action#75098
ThomThomson merged 9 commits intoelastic:masterfrom
ThomThomson:feature/addToLibraryAction

Conversation

@ThomThomson
Copy link
Copy Markdown
Contributor

@ThomThomson ThomThomson commented Aug 14, 2020 •
edited
Loading

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:

    kibana/src/plugins/dashboard/config.ts

    Line 23 in 5b64a4c

    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 Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// 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
Copy Markdown
Contributor

elasticmachine commented Aug 14, 2020

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

@ThomThomson ThomThomson mentioned this pull request Aug 14, 2020
Merged
2 tasks
majagrubic
majagrubic approved these changes Aug 18, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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:

kibana/src/plugins/dashboard/public/attribute_service/attribute_service.tsx

Line 178 in 0a0b8f2

reject();

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
Copy Markdown
Contributor Author

ThomThomson commented Aug 19, 2020

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
Copy Markdown
Contributor

kibanamachine commented Aug 19, 2020

💚 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

Dosant
Dosant approved these changes Aug 20, 2020
View reviewed changes
Copy link
Copy Markdown
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 ThomThomson mentioned this pull request Aug 20, 2020
Merged
ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Aug 20, 2020
@ThomThomson
[Dashboard First] Add to Library Action (elastic#75098)
1a35b78 
* 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
@ThomThomson
[Dashboard First] Add to Library Action (#75098) (#75562)
a618464 
* 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

Reviewers

@Dosant Dosant Dosant approved these changes

+1 more reviewer

@majagrubic majagrubic majagrubic approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature:Dashboard Dashboard related features Feature:Embedding Embedding content via iFrame Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.10.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ThomThomson @elasticmachine @Dosant @kibanamachine @majagrubic