-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Migrate doc view part of discover #58094
Conversation
Jenkins, test this. |
Pinging @elastic/kibana-app (Team:KibanaApp) |
@elasticmachine merge upstream |
There was a problem hiding this 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?
@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. |
There was a problem hiding this 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
All good, no need to adjust, thx for explanation 👍 |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
took another look, that was good to me, tested in Chrome, works |
There was a problem hiding this 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.
* 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) ...
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 thedocViews.addDocView
method exposed in the setup contract: