Skip to content

[Lens] Allow other apps to list their custom visualizations in the Visualize app#43303

Closed
chrisdavies wants to merge 4 commits intoelastic:feature/lensfrom
chrisdavies:lens/oss-visualize-app-extensions
Closed

[Lens] Allow other apps to list their custom visualizations in the Visualize app#43303
chrisdavies wants to merge 4 commits intoelastic:feature/lensfrom
chrisdavies:lens/oss-visualize-app-extensions

Conversation

@chrisdavies
Copy link
Contributor

This PR introduces a mechanism that allows other Kibana apps to register visualizations with the Visualize app. These visualizations have their own saved objects and their own separate editing experiences, but still show up in the Visualize app's visualization list and are added via the Visualize app's add button.

@chrisdavies chrisdavies added Feature:Vis Editor Visualization editor issues Feature:Lens labels Aug 14, 2019
@chrisdavies chrisdavies changed the title [Lens] Allow visualize app to be extended by other apps [Lens] Allow other apps to list their custom visualizations in the Visualize app Aug 14, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@wylieconlon
Copy link
Contributor

I was expecting the base branch to be master, not feature/lens- is that intentional?

@timroes
Copy link
Contributor

timroes commented Aug 15, 2019

I agree with @wylieconlon, the change to the vis listing should go into master so we don't run async there. But it seems this PR also contains the lens mapping changes (which most likely should be a separate PR)?

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Noticed a few small things (assumed the target branch and mapping changes went in by accident and only reviewed the OSS code)

defaultMessage: 'Error deleting visualization',
}),
});
this.deleteSelectedItems = function deleteSelectedItems(_ids, selectedItems) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be missing something, but can't we get rid of the _ids parameter? If not it should be documented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the current signature, so removing it would require changing all callers. (We're not the only caller, and there may be 3rd party plugins using it, too.) So, I only made backward-compatible changes.

this.deleteSelectedItems = function deleteSelectedItems(_ids, selectedItems) {
return Promise.all(
selectedItems.map(item => {
return savedObjectClient.delete(item.savedObjectType, item.id);
Copy link
Contributor

@flash1293 flash1293 Aug 15, 2019

Choose a reason for hiding this comment

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

The "old" delete function also called chrome.untrackNavLinksForDeletedSavedObjects in src/legacy/ui/public/saved_objects/saved_object_loader.js line 71, we probably also have to do that, even though I'm not sure what it does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. Good catch. Here's the comment for that function:
Clear last url for deleted saved objects to avoid loading pages with "Could not locate..."

Sounds like something we need.

)
},
{
field: 'canEdit',
Copy link
Contributor

Choose a reason for hiding this comment

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

The old "edit" link was gray as long as not hovered and included a tooltip - we should make sure to make this look identical:

Before:
Screenshot 2019-08-15 at 14 07 47

After:
Screenshot 2019-08-15 at 14 08 43

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm. Righto.

* under the License.
*/

import { findListItems } from './find_list_items';
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome to have tests for this 👍

@chrisdavies
Copy link
Contributor Author

OH. Weird. I didn't intend to include any x-pack stuff in this PR. I'll back it out, and re-submit against master and without the x-pack changes.

@chrisdavies
Copy link
Contributor Author

...OK. Closing this for (hopefully) the real PR which can be found here:

#43386

@chrisdavies chrisdavies deleted the lens/oss-visualize-app-extensions branch August 15, 2019 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Lens Feature:Vis Editor Visualization editor issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants