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

[Monitoring] Replace old kbn-top-nav with new one #43187

Merged
merged 2 commits into from
Aug 15, 2019
Merged

[Monitoring] Replace old kbn-top-nav with new one #43187

merged 2 commits into from
Aug 15, 2019

Conversation

chrisronline
Copy link
Contributor

@chrisronline chrisronline commented Aug 13, 2019 •
edited
Loading

Relates to #39981
Blocked by #43255

This PR removes the kbn-top-nav and replaces it with the new kbn-top-nav-v2. There is no functional change - everything should work as expected.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@chrisronline chrisronline marked this pull request as ready for review August 13, 2019 18:44
@chrisronline chrisronline self-assigned this Aug 13, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/stack-monitoring

@lizozom lizozom added the release_note:skip Skip the PR/issue when compiling release notes label Aug 14, 2019
lizozom
lizozom approved these changes Aug 14, 2019
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.

Tested. LGTM.

lizozom pushed a commit to lizozom/kibana that referenced this pull request Aug 14, 2019
Remove CSS import
1e2f1a2 
Removed fix to config in monitoring (fix is on elastic#43187)
This was referenced Aug 14, 2019
Closed
Merged
@ycombinator
Copy link
Contributor

ycombinator commented Aug 14, 2019 •
edited
Loading

On the very first page that I loaded up in the Stack Monitoring UI (with this PR), I noticed that the time picker had moved to a new line the left:

Screen Shot 2019-08-14 at 8 22 51 AM

Is this expected? It's not the same behavior on master (i.e. without this PR):

Screen Shot 2019-08-14 at 8 34 53 AM

@lizozom
Copy link
Contributor

lizozom commented Aug 14, 2019

@ycombinator it's a known issue of the new top nav ATM
#41900

There's already a PR that fixes it here
#43255

@ycombinator
Copy link
Contributor

ycombinator commented Aug 14, 2019 •
edited
Loading

Ah, perfect, thanks @lizozom! I'll ignore that particular issue in this PR for now and continue reviewing.

[EDIT] I do think we should block on merging this PR until #43255 is merged first.

@lizozom
Copy link
Contributor

lizozom commented Aug 14, 2019 •
edited
Loading

It's an issue across multiple apps (Visualizations, Timelion and Monitoring, now that you're using the same component) and the solution will be merged anyway.

If possible, don't block on it @ycombinator as the timeline for 7.4 is getting quite tight and I have another, harder to review, PR blocking on this one #43168

However, I guess the delay won't be more than a day or two anyway.

Like they say, as you fish 🐟

@chrisronline
Copy link
Contributor Author

@lizozom I'm with @ycombinator on this one. I think practically speaking it should be fine to merge, but I'd rather lean towards not merging something we know will negatively impact our users. While not likely, it's theoretically possible your PR isn't merged in time and I wouldn't want to ship this change in 7.4 with that bug outstanding.

I'm going to wait until #43255 is merged before merging this.

@lizozom
Copy link
Contributor

lizozom commented Aug 15, 2019

#43255 is merged :-)

@chrisronline chrisronline requested review from a team as code owners August 15, 2019 16:10
@chrisronline chrisronline requested review from a team August 15, 2019 16:10
@chrisronline chrisronline requested a review from a team as a code owner August 15, 2019 16:10
@chrisronline chrisronline removed request for a team August 15, 2019 16:12
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

ycombinator
ycombinator approved these changes Aug 15, 2019
View reviewed changes
Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

LGTM. I clicked around the Stack Monitoring application and watched the changes in the top nav. I compared the behavior on this PR with that on master. Everything looked and behaved the same AFAICT.

@chrisronline chrisronline merged commit 0751c8f into elastic:master Aug 15, 2019
@chrisronline chrisronline deleted the monitoring/new_top_nav branch August 15, 2019 19:31
@chrisronline
Copy link
Contributor Author

Hey @igoristic, I didn't want to miss your review, but I wanted to get this in so it unblocks @lizozom

chrisronline added a commit to chrisronline/kibana that referenced this pull request Aug 15, 2019
@chrisronline
[Monitoring] Replace old kbn-top-nav with new one (elastic#43187)
2fe6fd2 
* Replace old kbn-top-nav with new one

* Fix up our implementation
@chrisronline chrisronline mentioned this pull request Aug 15, 2019
Merged
chrisronline added a commit that referenced this pull request Aug 15, 2019
@chrisronline
[Monitoring] Replace old kbn-top-nav with new one (#43187) (#43400)
d2507ce 
* Replace old kbn-top-nav with new one

* Fix up our implementation
@chrisronline
Copy link
Contributor Author

Backport:

7.x: d2507ce

lizozom pushed a commit that referenced this pull request Aug 19, 2019
Deleted old kbn-top-nav directive (#43168)
c05598f 
* Deleted old kbn-top-nav directive.
Can be merged only when monitoring replace their last usage.

* fix texts

* Remove CSS import
Removed fix to config in monitoring (fix is on #43187)

* rename monitoring directive
lizozom pushed a commit to lizozom/kibana that referenced this pull request Aug 19, 2019
Deleted old kbn-top-nav directive (elastic#43168)
db03ff1 
* Deleted old kbn-top-nav directive.
Can be merged only when monitoring replace their last usage.

* fix texts

* Remove CSS import
Removed fix to config in monitoring (fix is on elastic#43187)

* rename monitoring directive
lizozom pushed a commit that referenced this pull request Aug 19, 2019
Deleted old kbn-top-nav directive (#43168) (#43523)
4849d03 
* Deleted old kbn-top-nav directive.
Can be merged only when monitoring replace their last usage.

* fix texts

* Remove CSS import
Removed fix to config in monitoring (fix is on #43187)

* rename monitoring directive
@chrisronline chrisronline mentioned this pull request Apr 16, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lizozom lizozom lizozom approved these changes

@ycombinator ycombinator ycombinator approved these changes

@igoristic igoristic Awaiting requested review from igoristic

Assignees

@chrisronline chrisronline

Labels
release_note:skip Skip the PR/issue when compiling release notes review Team:Monitoring Stack Monitoring team v7.4.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chrisronline @elasticmachine @ycombinator @lizozom