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.

Sign up for GitHub

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

Jump to bottom

[NP] Move ui/kbn_top_nav/kbn_top_nav to kibana_legacy #58221

Merged
merged 7 commits into from
Feb 26, 2020
Merged

[NP] Move ui/kbn_top_nav/kbn_top_nav to kibana_legacy #58221

merged 7 commits into from
Feb 26, 2020

Conversation

maryia-lapata
Copy link
Contributor

Part of #57180.

Movement src/legacy/ui/public/kbn_top_nav/kbn_top_nav.js to src/plugins/kibana_legacy.

@maryia-lapata maryia-lapata added Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:NP Migration v7.7.0 labels Feb 21, 2020
@elasticmachine
Copy link
Contributor

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

flash1293
flash1293 requested changes Feb 21, 2020
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

If you check out the implementation of wrapInI18nContext over in src/legacy/ui/public/i18n/index.tsx, you will see it's just putting the I18nContext component from core start around the component. Introducing reliance on this function in several places feels like a step in the wrong direction.

It seems like everything would get much easier if the navigation plugin would already do that. As the component itself is already returned from the start contract, it would be trivial to wrap it in coreStart.i18n.Context right there: src/plugins/navigation/public/plugin.ts

@lizozom what do you think?

@maryia-lapata
Copy link
Contributor Author

@elasticmachine merge upstream

@maryia-lapata
Copy link
Contributor Author

@elasticmachine merge upstream

@maryia-lapata maryia-lapata marked this pull request as ready for review February 25, 2020 08:29
@maryia-lapata maryia-lapata requested a review from a team February 25, 2020 08:29
@maryia-lapata maryia-lapata requested a review from a team as a code owner February 25, 2020 08:29
@maryia-lapata maryia-lapata requested a review from a team February 25, 2020 08:29
lizozom
lizozom reviewed Feb 25, 2020
View reviewed changes
Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

Overall the change looks great.
I would suggest removing the ui/public folder altogether, as it won't require much more additional work.

src/legacy/ui/public/kbn_top_nav/kbn_top_nav.js Outdated
import { uiModules } from 'ui/modules';
import { npStart } from 'ui/new_platform';
import {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets move the directive definition to kibana_legacy and update the imports from the 4 places that consume those directives (maps, dashboard_mode, timelion and kibana).

Then we'll be able to remove this folder altogether and simplify the code.

@maryia-lapata
Move the kbnTopNav directive definition to kibana_legacy and remove u…
fb684d9 
…i/kbn_top_nav
@maryia-lapata
Merge remote-tracking branch 'kibana/master' into move-krn-top-nav
0410b51 
# Conflicts:
#	x-pack/legacy/plugins/graph/public/application.ts
#	x-pack/legacy/plugins/graph/public/legacy_imports.ts
@maryia-lapata maryia-lapata requested review from a team as code owners February 25, 2020 12:04
pgayvallet
pgayvallet approved these changes Feb 25, 2020
View reviewed changes
Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

LGTM for migration.md

lizozom
lizozom approved these changes Feb 25, 2020
View reviewed changes
Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

flash1293
flash1293 approved these changes Feb 25, 2020
View reviewed changes
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.

LGTM, did not test locally

nreese
nreese approved these changes Feb 25, 2020
View reviewed changes
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

maps changes LGTM

@lizozom lizozom mentioned this pull request Feb 26, 2020
Closed
8 tasks
igoristic
igoristic approved these changes Feb 26, 2020
View reviewed changes
Copy link
Contributor

@igoristic igoristic left a comment

Choose a reason for hiding this comment

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

Everything looks good on the Monitoring side. Great job 👍

@maryia-lapata maryia-lapata merged commit 55fb05c into elastic:master Feb 26, 2020
@maryia-lapata maryia-lapata deleted the move-krn-top-nav branch February 26, 2020 17:12
@maryia-lapata maryia-lapata mentioned this pull request Feb 26, 2020
Merged
maryia-lapata added a commit that referenced this pull request Feb 27, 2020
@maryia-lapata @elasticmachine
[NP] Move ui/kbn_top_nav/kbn_top_nav to kibana_legacy (#58221) (#58620)
11df8d6 
* Migrate kbn_top_nav.js to kibana_legacy

* Wrap TopNavMenu into i18nContext

* Move the kbnTopNav directive definition to kibana_legacy and remove ui/kbn_top_nav

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igoristic igoristic igoristic approved these changes

@lizozom lizozom lizozom approved these changes

@pgayvallet pgayvallet pgayvallet approved these changes

@flash1293 flash1293 flash1293 approved these changes

@nreese nreese nreese approved these changes

Assignees
No one assigned
Labels
backported Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.7.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@maryia-lapata @elasticmachine @kibanamachine @nreese @flash1293 @pgayvallet @lizozom @igoristic