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

[Discover] Centralize dependencies in kibana_services.ts #48072

Merged
merged 52 commits into from
Oct 25, 2019

Conversation

kertal
Copy link
Member

@kertal kertal commented Oct 14, 2019

Summary

This PR moves most angular code / dependencies into the angular subfolder, centralizes dependencies in kibana_services.ts and implements a basic new platform plugin structure

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [ ] This was checked for cross-browser compatibility, including a check against IE11
- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
- [ ] Documentation was added for features that require explanation or tutorials
- [ ] Unit or functional tests were updated or added to match the most common scenarios
- [ ] This was checked for keyboard-only and screenreader accessibility

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately

@kertal kertal self-assigned this Oct 14, 2019
@kertal kertal added Feature:Graph Graph application feature Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes v8.0.0 and removed Feature:Graph Graph application feature labels Oct 14, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@kertal kertal mentioned this pull request Oct 14, 2019
46 tasks
@kertal kertal changed the title [Discover] New platform migration structural adaption [Discover] Centralize dependencies in kibana_services.ts Oct 21, 2019
@kertal kertal added the v7.6.0 label Oct 21, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elastic elastic deleted a comment from elasticmachine Oct 24, 2019
@kertal kertal requested a review from flash1293 October 24, 2019 07:22
@kertal kertal marked this pull request as ready for review October 24, 2019 07:42
@kertal kertal requested review from a team as code owners October 24, 2019 07:42
import { VISUALIZE_EMBEDDABLE_TYPE } from '../../../../../../src/legacy/core_plugins/kibana/public/visualize/embeddable';
import { SEARCH_EMBEDDABLE_TYPE } from '../../../../../../src/legacy/core_plugins/kibana/public/discover/embeddable/constants';
Copy link
Member Author

Choose a reason for hiding this comment

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

dear @elastic/kibana-canvas had to change this import, to ensure, that just this const is imported, and not all of our centralized dependencies of our Discover's NP refactoring

FYI: @flash1293 that's what caused the jest test to fail

@@ -23,4 +23,4 @@
@import 'doc_viewer/index';

// Context styles
@import 'context/index';
@import 'angular/context/index';
Copy link
Member Author

Choose a reason for hiding this comment

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

Dear @elastic/kibana-design , valued owner and ruler of the 7 SCSS code kingdoms
There were just path changes in this PR.
Your humble servant
@kertal

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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.

Looks good to me, just once place in the embeddable where we can remove a reference to the new platform.

import { TExecuteTriggerActions } from 'src/plugins/ui_actions/public';
import { npStart, npSetup } from 'ui/new_platform';
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like kibana_services can provide those.

Copy link
Member Author

Choose a reason for hiding this comment

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

@flash1293 thx, yes, I've applied the changes, and tests look now good again 🎉

@elasticmachine
Copy link
Contributor

💔 Build Failed

@kertal
Copy link
Member Author

kertal commented Oct 25, 2019

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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.

Canvas changes are good 👍

@kertal kertal merged commit f104e23 into elastic:master Oct 25, 2019
@kertal kertal added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Dec 4, 2019
@elasticmachine
Copy link
Contributor

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

@kertal kertal deleted the kertal-pr-2019-10-10-np-basic-structure branch July 6, 2020 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Discover Discover Application Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants