Skip to content

Migrate doc view part of discover#58094

Merged
flash1293 merged 19 commits intoelastic:masterfrom
flash1293:migrate-docview
Mar 24, 2020
Merged

Migrate doc view part of discover#58094
flash1293 merged 19 commits intoelastic:masterfrom
flash1293:migrate-docview

Conversation

@flash1293
Copy link
Copy Markdown
Contributor

@flash1293 flash1293 commented Feb 20, 2020 •
edited
Loading

This PR moves the doc views part of Discover into the NP discover plugin. To do so, a few things have to be temporary exposed to enable legacy and NP part of discover to talk to each other.

This also adds a test plugin unit-testing the doc views integration for both angular and react.

The main purpose of this PR is to have the doc views API in its final place for 7.7 to enable developers to use it from the new platform directly.

Dev docs

The extension point for registering custom doc views was migrateed and can be used directly within the new platform.

An working example of the new integration can be seen in test/plugin_functional/plugins/doc_views_plugin/public/plugin.tsx.

To register doc views, list discover as a required dependency of your plugin and use the docViews.addDocView method exposed in the setup contract:

export class MyPlugin implements Plugin<void, void> {
  public setup(core: CoreSetup, { discover }: { discover: DiscoverSetup }) {
    discover.docViews.addDocView({
      component: props => {
        return /* ... */;
      },
      order: 2,
      title: 'My custom doc view',
    });
  }

  /* ... */
}

@flash1293
Copy link
Copy Markdown
Contributor Author

flash1293 commented Feb 21, 2020

Jenkins, test this.

@flash1293 flash1293 marked this pull request as ready for review March 11, 2020 10:35
@flash1293 flash1293 requested a review from a team March 11, 2020 10:35
@flash1293 flash1293 requested review from a team as code owners March 11, 2020 10:35
@flash1293 flash1293 added Feature:Discover Discover Application Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.7.0 v8.0.0 labels Mar 11, 2020
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Mar 11, 2020

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

@flash1293 flash1293 added the release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. label Mar 11, 2020
@kertal
Copy link
Copy Markdown
Member

kertal commented Mar 13, 2020

@elasticmachine merge upstream

kertal
kertal approved these changes Mar 14, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@kertal kertal 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, tested locally in Chrome, works. Great that there's now also a functional test using the angular legacy way of registering doc views 👍

One thing that came to my mind, to discuss. Wouldn't it be a simpler approach to export the registry in the plugin and DocViewer as static export, so DocViewer would retrieve the registry as property?

@flash1293
Copy link
Copy Markdown
Contributor Author

flash1293 commented Mar 15, 2020 •
edited
Loading

@kertal The reason I set up the exposed API like this is to make it easy to remove the parts which become irrelevant once we migrate the rest of discover (set angular injector and the DocViewer component) - they are just meant to be there for a short transition period and will become an implementation detail of the completely migrated discover plugin in the future.

Right now, we just have to remove the angular injector getter and the DocView component - if the whole registry instance is exposed, it's slightly more work later to revert back and also slightly harder to make clear which APIs are there to stay and which APIs are just temporary. Not a big deal though, if you like the static export and exposed registry better, I can also adjust, no strong opinion on this.

joshdover
joshdover approved these changes Mar 15, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

Migration guide changes LGTM

@kertal
Copy link
Copy Markdown
Member

kertal commented Mar 16, 2020

Not a big deal though, if you like the static export and exposed registry better, I can also adjust, no strong opinion on this.

All good, no need to adjust, thx for explanation 👍

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Mar 23, 2020

💚 Build Succeeded

History

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

@kertal
Copy link
Copy Markdown
Member

kertal commented Mar 24, 2020

took another look, that was good to me, tested in Chrome, works

snide
snide approved these changes Mar 24, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Review Sass changes only. Looks like a simple NP file move on the sass side.

@flash1293 flash1293 merged commit 3e26777 into elastic:master Mar 24, 2020
@flash1293 flash1293 mentioned this pull request Mar 24, 2020
Merged
flash1293 added a commit to flash1293/kibana that referenced this pull request Mar 24, 2020
@flash1293
Migrate doc view part of discover (elastic#58094)
a18a9b0
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 24, 2020
@gmmorris
Merge branch 'master' into alerting/tls-warning
1b3d0b4 
* master: (34 commits)
  [APM] add service map config options to legacy plugin (elastic#61002)
  [App Arch] migrate legacy CSS to new platform (core_plugins/kibana_react) (elastic#59882)
  Migrated styles for "share" plugin to new platform (elastic#59981)
  [ML] Module setup with dynamic model memory estimation (elastic#60656)
  Drilldowns (elastic#59632)
  Upgrade mocha dev-dependency from 6.2.2 to 7.1.1 (elastic#60779)
  [SIEM] Overview: Recent cases widget (elastic#60993)
  [ML] Functional tests - stabilize df analytics clone tests (elastic#60497)
  [SIEM] Updates process and TLS tables to use ECS 1.5 fields (elastic#60854)
  Migrate doc view part of discover (elastic#58094)
  Revert "[APM] Collect telemetry about data/API performance (elastic#51612)"
  fix(NA): log rotation watchers usage (elastic#60956)
  [SIEM] [CASES] Build lego blocks case details view (elastic#60864)
  Create Painless Lab app (elastic#57538)
  [SIEM] Move Timeline Template field to first step of rule creation (elastic#60840)
  [Reporting/New Platform Migration] Use a new config service on server-side (elastic#55882)
  [Alerting] allow email action to not require auth (elastic#60839)
  [Maps] Default ES document layer scaling type to clusters and show scaling UI in the create wizard (elastic#60668)
  [APM] Collect telemetry about data/API performance (elastic#51612)
  Implement Kibana Login Selector (elastic#53010)
  ...
flash1293 added a commit that referenced this pull request Mar 24, 2020
@flash1293
Migrate doc view part of discover (#58094) (#61033)
7d32617
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kertal kertal kertal approved these changes

+2 more reviewers

@snide snide snide approved these changes

@joshdover joshdover joshdover approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature:Discover Discover Application release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.7.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@flash1293 @elasticmachine @kertal @kibanamachine @snide @joshdover