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

[k7/Infra UI] Integrate with K7 Breadcrumbs #25938

Conversation

weltenwort
Copy link
Member

@weltenwort weltenwort commented Nov 20, 2018

This PR changes the header to be conditionally rendered based on the k7design UI setting. If the setting is false, the header is rendered as before. If it is true, the header is hidden and the breadcrumbs are set via the Kibana breadcrumbs api.

image
image
image

Notes

Testing

  • Ensure the k7design UI settings is disabled.
  • Check that the old header is rendered.
  • Enable the k7design UI setting.
  • Check that the K7 Header with correct breadcrumbs is rendered on the home page, the node detail page and the log page.

Related Issues

@weltenwort weltenwort self-assigned this Nov 20, 2018
@elasticmachine
Copy link
Contributor

Pinging @elastic/infrastructure-ui

@weltenwort weltenwort changed the title [Infra UI] Integrate with K7 Breadcrumbs [k7/Infra UI] Integrate with K7 Breadcrumbs Nov 20, 2018
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@spalger
Copy link
Contributor

spalger commented Nov 20, 2018

Merged #25914 this morning, would you mind updating this to use the new chrome.breadcrumbs service? Also, would you mind describing the breadcrumb hierarchy for me (or we can walk through it in a call if you like)?

@weltenwort weltenwort force-pushed the infra-ui-enhancement-integrate-k7-breadcrumbs branch from 4f86be7 to f3dc081 Compare November 21, 2018 10:11
@weltenwort
Copy link
Member Author

@spalger rebased on master and updated to use chrome.breadcrumbs. To do so properly I had to add the breadcrumbs property to the Chrome type. Since the breadcrumbs api itself is already written in TypeScript, I figured I could avoid spreading the type defs around by turning Chrome into an interface (it wasn't a class anyway) and pulling in the types from the api/breadcrumbs file. What do you think?

Regarding the breadcrumbs hierarchy, it's quite simple (maybe too simple). Every page just renders a <Header> with a sequence of breadcrumbs. There currently only are

  • Infrastructure
  • Infrastructure / Logs
  • Infrastructure / ${nodeId}

I didn't what to mix in any changes to the hierarchy here to limit the scope of this PR.

@weltenwort
Copy link
Member Author

Are the K7 header changes supposed to be backported to 6.x?

@elasticmachine
Copy link
Contributor

💔 Build Failed

@weltenwort
Copy link
Member Author

weltenwort commented Nov 21, 2018

same failure as on master (#26000)

[00:12:17]                   └- ✖ fail: "visualize app  visual builder switch index patterns should be able to switch between index patterns"
[00:12:17]                   │ 
[00:12:17]                   │       Error: expected '0' to sort of equal '10'
[00:12:17]                   │       + expected - actual
[00:12:17]                   │ 
[00:12:17]                   │       -0
[00:12:17]                   │       +10
[00:12:17]                   │       
[00:12:17]                   │       at Assertion.assert (node_modules/expect.js/index.js:96:13)
[00:12:17]                   │       at Assertion.eql (node_modules/expect.js/index.js:230:10)
[00:12:17]                   │       at Context.it (test/functional/apps/visualize/_tsvb_chart.js:238:29)
[00:12:17]                   │       at <anonymous>
[00:12:17]                   │       at process._tickCallback (internal/process/next_tick.js:188:7)

@spalger
Copy link
Contributor

spalger commented Nov 21, 2018

I have #25973 in to fix that @weltenwort. Jenkins, would you please test this?

@elastic elastic deleted a comment from elasticmachine Nov 21, 2018
@elastic elastic deleted a comment from elasticmachine Nov 21, 2018
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@skh skh left a comment

Choose a reason for hiding this comment

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

Ensure the k7design UI settings is disabled.
Check that the old header is rendered.

👍

Enable the k7design UI setting.
Check that the K7 Header with correct breadcrumbs is rendered on the home page, the node detail page and the log page.

👍

Works as described. Code looks good.

@weltenwort weltenwort force-pushed the infra-ui-enhancement-integrate-k7-breadcrumbs branch from f3dc081 to 175d0ae Compare November 28, 2018 15:22
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@weltenwort
Copy link
Member Author

@tbragin given that the new header is behind a feature switch in the advanced settings, are we ok to merge this without the BETA badge being visible in K7 mode? Or do we want to find a different location for it before merging?

@tbragin
Copy link
Contributor

tbragin commented Nov 30, 2018

I think we are ok w.o a beta label in K7, assuming we remain on track to GA Logs and Infrastructure before or in 7.0. If we don't meet that deadline, we could always add it back. @makwarth Do you have thoughts on this?

@makwarth
Copy link

makwarth commented Dec 3, 2018

@tbragin Agree 👍

@weltenwort weltenwort merged commit ddd243d into elastic:master Dec 3, 2018
weltenwort added a commit to weltenwort/kibana that referenced this pull request Dec 3, 2018
This changes the header to be conditionally rendered based on the k7design UI setting. If the setting is false, the header is rendered as before. If it is true, the header is hidden and the breadcrumbs are set via the Kibana breadcrumbs api.
weltenwort added a commit to weltenwort/kibana that referenced this pull request Dec 3, 2018
This changes the header to be conditionally rendered based on the k7design UI setting. If the setting is false, the header is rendered as before. If it is true, the header is hidden and the breadcrumbs are set via the Kibana breadcrumbs api.
weltenwort added a commit that referenced this pull request Dec 3, 2018
This changes the header to be conditionally rendered based on the k7design UI setting. If the setting is false, the header is rendered as before. If it is true, the header is hidden and the breadcrumbs are set via the Kibana breadcrumbs api.
@spalger
Copy link
Contributor

spalger commented Dec 3, 2018

@weltenwort was there a follow up issue to change which breadcrumbs are in use to follow the guidelines in #25689?

@weltenwort
Copy link
Member Author

no, I was not aware of that. should be a simple task for the Infrastructure UI, but not necessarily for the other solutions. what is the plan and timeline to getting the solutions teams involved?

@spalger
Copy link
Contributor

spalger commented Dec 3, 2018

We'd really like to get the breadcrumbs all updated for alpha2, so if you or someone else has some time to throw at it this week that'd be great

@weltenwort
Copy link
Member Author

ok, tracking it in #26582

@elasticmachine
Copy link
Contributor

💔 Build Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants