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

Migrate vis_type_table to kibana/new platform #63105

Merged

Conversation

kertal
Copy link
Member

@kertal kertal commented Apr 9, 2020

Summary

This PR moves the Table Visualization to kibana/new platform.

Part of #60097

@kertal kertal requested a review from a team April 9, 2020 12:22
@kertal kertal requested a review from a team as a code owner April 9, 2020 12:22
@kertal kertal marked this pull request as draft April 9, 2020 12:22
@kertal kertal self-assigned this Apr 9, 2020
@flash1293 flash1293 mentioned this pull request Apr 9, 2020
69 tasks
@kertal kertal added v8.0.0 v7.8.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Apr 9, 2020
@elasticmachine
Copy link
Contributor

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

@kertal kertal added the release_note:skip Skip the PR/issue when compiling release notes label Apr 9, 2020
@kertal kertal force-pushed the kertal-pr-2020-04-09-vis_type_table-kibana-platform branch from f171a3a to bcc5de1 Compare April 10, 2020 13:11
@kertal kertal marked this pull request as ready for review April 12, 2020 20:48
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.

Left a comment about setup/start phase usage. Also, this PR is having the same problem as #63096 (review)

description: i18n.translate('visTypeTable.tableVisDescription', {
defaultMessage: 'Display values in a table',
}),
visualization: getTableVisualizationControllerClass(core, context),
Copy link
Contributor

Choose a reason for hiding this comment

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

Woa, good catch, I completely missed it directly imported the start contract in the old version - super helpful. It seems a bit dangerous though to move the visualization registration into start here: https://github.com/elastic/kibana/pull/63105/files#diff-381e25468de5bf2e485a1957d63c9b07R62

I followed the call stack and it seems this is just necessary because of initLocalAngular here: https://github.com/elastic/kibana/blob/master/src/legacy/core_plugins/vis_type_table/public/vis_controller.ts#L57

But the local angular is only initialized in the render function (https://github.com/elastic/kibana/blob/master/src/legacy/core_plugins/vis_type_table/public/vis_controller.ts#L63), so we can do the registration in the setup phase, pass down the CoreSetup contract and then in render get CoreStart via await coreSetup.getStartServices(). This way we are doing everything in the "right" phase

Copy link
Member Author

Choose a reason for hiding this comment

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

many thx @flash1293 , adapted it that way

@@ -7,6 +7,6 @@
// tbvChart__legend--small
// tbvChart__legend-isLoading

@import 'agg_table/index';
Copy link
Contributor

Choose a reason for hiding this comment

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

The styling_constants import at the top of this file is no longer necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

thx @cchaos, I've removed it

@kertal kertal requested review from cchaos and flash1293 April 17, 2020 07:41
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

SASS LGTM, thanks!

@kertal
Copy link
Member Author

kertal commented Apr 20, 2020

@elasticmachine merge upstream

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.

Tested in chrome, everything is rendered correctly, filters still work, all tests seem to run successfully, code structure is sound. Great work, LGTM 🚀

}

public start(core: CoreStart, { data }: TablePluginStartDependencies) {
public start(core: CoreStart, { data, visualizations }: TablePluginStartDependencies) {
Copy link
Contributor

Choose a reason for hiding this comment

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

unused leftover

Copy link
Member Author

Choose a reason for hiding this comment

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

thx! removed it!

kertal added 2 commits April 21, 2020 07:59
… github.com:kertal/kibana into kertal-pr-2020-04-09-vis_type_table-kibana-platform
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@kertal kertal merged commit 2f363ba into elastic:master Apr 21, 2020
kertal added a commit to kertal/kibana that referenced this pull request Apr 21, 2020
* Move vis_type_table to Kibana Platform

* Adapt mocha tests
* Adapt CODEOWNERS

* Adapt SCSS
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 21, 2020
* master: (30 commits)
  Migrate vis_type_table to kibana/new platform  (elastic#63105)
  Enable include/exclude in Terms agg for numeric fields (elastic#59425)
  follow conventions for saved object definitions (elastic#63571)
  [Docs]Adds saved object key setting to load balancing kib instances (elastic#63935)
  kbn/config-schema: Consider maybe properties as optional keys in ObjectType (elastic#63838)
  Fix page layouts, clean up unused code (elastic#63992)
  [SIEM] Adds recursive exact key checks for validation and formatter
  [Maps] Migrate remaining maps client files to NP (except routi… (elastic#63859)
  [Maps] Do not fetch geo_shape from docvalues (elastic#63997)
  Vega doc changes (elastic#63889)
  [Metrics UI] Reorganize file layout for Metrics UI (elastic#60049)
  Add sub-menus to Resolver node (for 75% zoom) (elastic#63476)
  [FTR] Add test suite metrics tracking/output (elastic#62515)
  [ML] Fixing single metric viewer page padding (elastic#63839)
  [Discover] Allow user to generate a report after saving a modified saved search (elastic#63623)
  [Reporting] Config flag to escape formula CSV values (elastic#63645)
  [Metrics UI] Remove remaining field filtering (elastic#63398)
  [Maps] fix date labels (elastic#63909)
  [TSVB] Use default Kibana palette for split series (elastic#62241)
  Ingest overview page (elastic#63214)
  ...
kertal added a commit that referenced this pull request Apr 21, 2020
* Move vis_type_table to Kibana Platform

* Adapt mocha tests

* Adapt SCSS
jloleysens added a commit that referenced this pull request Apr 21, 2020
…t-node-pipelines

* 'master' of github.com:elastic/kibana: (32 commits)
  Move authz lib out of snapshot restore (#63947)
  Migrate vis_type_table to kibana/new platform  (#63105)
  Enable include/exclude in Terms agg for numeric fields (#59425)
  follow conventions for saved object definitions (#63571)
  [Docs]Adds saved object key setting to load balancing kib instances (#63935)
  kbn/config-schema: Consider maybe properties as optional keys in ObjectType (#63838)
  Fix page layouts, clean up unused code (#63992)
  [SIEM] Adds recursive exact key checks for validation and formatter
  [Maps] Migrate remaining maps client files to NP (except routi… (#63859)
  [Maps] Do not fetch geo_shape from docvalues (#63997)
  Vega doc changes (#63889)
  [Metrics UI] Reorganize file layout for Metrics UI (#60049)
  Add sub-menus to Resolver node (for 75% zoom) (#63476)
  [FTR] Add test suite metrics tracking/output (#62515)
  [ML] Fixing single metric viewer page padding (#63839)
  [Discover] Allow user to generate a report after saving a modified saved search (#63623)
  [Reporting] Config flag to escape formula CSV values (#63645)
  [Metrics UI] Remove remaining field filtering (#63398)
  [Maps] fix date labels (#63909)
  [TSVB] Use default Kibana palette for split series (#62241)
  ...
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 21, 2020
…bana into ingest-node-pipelines/privileges

* 'feature/ingest-node-pipelines' of github.com:elastic/kibana: (34 commits)
  Move authz lib out of snapshot restore (elastic#63947)
  Migrate vis_type_table to kibana/new platform  (elastic#63105)
  Enable include/exclude in Terms agg for numeric fields (elastic#59425)
  follow conventions for saved object definitions (elastic#63571)
  [Docs]Adds saved object key setting to load balancing kib instances (elastic#63935)
  kbn/config-schema: Consider maybe properties as optional keys in ObjectType (elastic#63838)
  Fix page layouts, clean up unused code (elastic#63992)
  [SIEM] Adds recursive exact key checks for validation and formatter
  [Maps] Migrate remaining maps client files to NP (except routi… (elastic#63859)
  [Maps] Do not fetch geo_shape from docvalues (elastic#63997)
  Vega doc changes (elastic#63889)
  [Metrics UI] Reorganize file layout for Metrics UI (elastic#60049)
  Add sub-menus to Resolver node (for 75% zoom) (elastic#63476)
  [FTR] Add test suite metrics tracking/output (elastic#62515)
  [Ingest pipelines] Delete pipeline (elastic#63635)
  [ML] Fixing single metric viewer page padding (elastic#63839)
  [Discover] Allow user to generate a report after saving a modified saved search (elastic#63623)
  [Reporting] Config flag to escape formula CSV values (elastic#63645)
  [Metrics UI] Remove remaining field filtering (elastic#63398)
  [Maps] fix date labels (elastic#63909)
  ...

# Conflicts:
#	x-pack/legacy/plugins/uptime/public/components/monitor/ml/index.ts
#	x-pack/legacy/plugins/uptime/public/components/overview/index.ts
#	x-pack/plugins/ingest_pipelines/public/application/index.tsx
#	x-pack/plugins/ingest_pipelines/server/routes/api/index.ts
#	x-pack/plugins/ingest_pipelines/server/routes/index.ts
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 22, 2020
…bana into pipeline-editor-part-mvp-2

* 'feature/ingest-node-pipelines' of github.com:elastic/kibana: (34 commits)
  [Ingest Node Pipelines] Clone Pipeline (elastic#64049)
  Move authz lib out of snapshot restore (elastic#63947)
  Migrate vis_type_table to kibana/new platform  (elastic#63105)
  Enable include/exclude in Terms agg for numeric fields (elastic#59425)
  follow conventions for saved object definitions (elastic#63571)
  [Docs]Adds saved object key setting to load balancing kib instances (elastic#63935)
  kbn/config-schema: Consider maybe properties as optional keys in ObjectType (elastic#63838)
  Fix page layouts, clean up unused code (elastic#63992)
  [SIEM] Adds recursive exact key checks for validation and formatter
  [Maps] Migrate remaining maps client files to NP (except routi… (elastic#63859)
  [Maps] Do not fetch geo_shape from docvalues (elastic#63997)
  Vega doc changes (elastic#63889)
  [Metrics UI] Reorganize file layout for Metrics UI (elastic#60049)
  Add sub-menus to Resolver node (for 75% zoom) (elastic#63476)
  [FTR] Add test suite metrics tracking/output (elastic#62515)
  [Ingest pipelines] Delete pipeline (elastic#63635)
  [ML] Fixing single metric viewer page padding (elastic#63839)
  [Discover] Allow user to generate a report after saving a modified saved search (elastic#63623)
  [Reporting] Config flag to escape formula CSV values (elastic#63645)
  [Metrics UI] Remove remaining field filtering (elastic#63398)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants