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

Setting up Presentation Util; Create Service Abstraction API #88112

Merged
merged 4 commits into from
Jan 28, 2021

Conversation

clintandrewhall
Copy link
Contributor

Summary

This PR completes a number of items:

  • Creates the Plugin Services model for use in Presentation projects;
  • Implements plugin services for the Presentation Utility plugin;
  • Refactors components to use plugin-provided services, (rather than expect the consuming plugin to provide them);
  • Adds Storybook to the Presentation Utility plugin;
  • Documents all of the above.

Canvas has a bespoke method of providing simplified services to the plugin, (rather than consume useKibana or similar, which are notoriously difficult to track/mock). This PR extracts and improves upon that approach to be used in Dashboard, Canvas and elsewhere, (PR next). The net benefit is that plugins consuming Presentation Utility components don't need to provide full implementations of Kibana services, (particularly if they weren't present to begin with). In other words, there's no need to add savedObjects to the kibana.json for your plugin just to consume presentationUtil.

While working on the new service architecture, I added and refactored to provide Storybook to the Presentation Utility plugin.

I also took the opportunity to write docs using the new docs system, (a PR to add the items to the docs is forthcoming).

Screen Shot 2021-01-12 at 6 28 04 PM

Screen Shot 2021-01-12 at 6 28 49 PM

@clintandrewhall clintandrewhall added review Feature:Dashboard Dashboard related features Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:medium Medium Level of Effort v8.0.0 release_note:skip Skip the PR/issue when compiling release notes impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. v7.12.0 Feature:Canvas labels Jan 12, 2021
@clintandrewhall clintandrewhall requested a review from a team January 12, 2021 23:31
@clintandrewhall clintandrewhall requested a review from a team as a code owner January 12, 2021 23:31
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@clintandrewhall clintandrewhall requested a review from a team January 12, 2021 23:32
@clintandrewhall
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@tylersmalley tylersmalley left a comment

Choose a reason for hiding this comment

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

Ops change looks good.

@crob611
Copy link
Contributor

crob611 commented Jan 14, 2021

Looks like this is breaking in Kibana if you go to Lens and try to save in a way that will bring up the dashboard_picker

The error is that registry hasn't been set. Looks like it's because your presentation util plugin is providing a "getServices" method, but because the dashboard_picker accesses services directly, nothing is ever starting the services.

Copy link
Contributor

@crob611 crob611 left a comment

Choose a reason for hiding this comment

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

Left a few thoughts, and the issue with the plugins not being started needs to be resolved, but the code as a whole looks good. This is going to be great to build on going forward.

src/plugins/visualize/kibana.json Show resolved Hide resolved
Copy link
Contributor

@stacey-gammon stacey-gammon left a comment

Choose a reason for hiding this comment

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

I love the concept, I'm just not sure if these interfaces/classes belong in the platform, or if we should consider this a recommended "best practice", and leave it up to each plugin author to build the service how they need.

Copy link
Contributor

@poffdeluxe poffdeluxe left a comment

Choose a reason for hiding this comment

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

Found a bug with the refactor of the modal but otherwise I'm fine with the setup

Copy link
Contributor

@poffdeluxe poffdeluxe left a comment

Choose a reason for hiding this comment

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

Looks good to me -- there's one fix you'll want to make in maps (I should've caught that... sorry..) but I'm approving to unblock 👍

Copy link
Contributor

@crob611 crob611 left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@stacey-gammon
Copy link
Contributor

Love seeing more mdx docs!! I'll add the Team:Docs label so this gets included in the docs challenge. One thing though - can you nest this under the Services section and make Plugin services toolkit a section that is one part of the docs for the entire Presentation plugin? The plan is to have one entry per plugin, or if more breakdown is needed, do something like data.indexPatterns to break it up (this is how I'm planning on breaking up the API documentation system since some plugins are just way too big.

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-docs (Team:Docs)

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

maps/** lgtm!

tagOptions={tagOptions}
objectType={'visualization'}
onClose={() => {}}
savedObjectsClient={savedObjectsClient}
Copy link
Contributor

@stratoula stratoula Jan 28, 2021

Choose a reason for hiding this comment

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

It seems that now savedObjectsClient it is not used for getTopNavConfig so no need to exist and it can be removed from the props here:

   export const getTopNavConfig = (
  {
    hasUnsavedChanges,
    setHasUnsavedChanges,
    openInspector,
    originatingApp,
    setOriginatingApp,
    hasUnappliedChanges,
    visInstance,
    stateContainer,
    savedObjectsClient,
    visualizationIdFromUrl,
    stateTransfer,
    embeddableId,
  }

It should be cleared from the visualize_top_nav.tsx too

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.

Lens changes LGTM

@clintandrewhall clintandrewhall merged commit 55afba4 into elastic:master Jan 28, 2021
@clintandrewhall clintandrewhall deleted the presentation_util branch May 28, 2021 04:43
@clintandrewhall clintandrewhall changed the title Setting up and documenting Presentation Util Setting up Presentation Util; Create Service Abstraction API Jun 25, 2021
@kibanamachine
Copy link
Contributor

kibanamachine commented Jun 25, 2021

💔 Build Failed

Failed CI Steps

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [96e4bdc]

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Canvas Feature:Dashboard Dashboard related features impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:medium Medium Level of Effort release_note:skip Skip the PR/issue when compiling release notes review Team:Docs Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants