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] implement k7Breadcrumbs #26587

Merged
merged 5 commits into from
Dec 4, 2018

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Dec 3, 2018

Fixes #25894

Summary

This PR updates the discover routes to provide k7Breadcrumbs used by the new header navigation. See #25884 for general information about the integration with the router and #25689 for the breadcrumb taxonomy

2018-12-03 15 32 14

Checklist

@spalger spalger added review v7.0.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Dec 3, 2018
@spalger spalger requested a review from cchaos December 3, 2018 23:31
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@cchaos
Copy link
Contributor

cchaos commented Dec 3, 2018

A couple questions:

  1. What is the relationship between the breadcrumb title, local title, and tab name here? Seems it can be simplified?
    image

  2. Shouldn't the single doc viewer also have the "Discover" breadcrumb?
    image

@spalger
Copy link
Contributor Author

spalger commented Dec 3, 2018

@cchaos re 1, I'm not sure. I tried putting the id, or the index, in the breadcrumb, but they both just got tucked into ellipses and then I kind of figured that "Context" kind of described this page better than "Surrounding ...".

re 2, yeah, it probably should.

@cchaos
Copy link
Contributor

cchaos commented Dec 4, 2018

  1. I think we can fix the truncation issue in EUI by not truncating the last item unless it touches the right side controls. I think it'll be nice to put that title up in the breadcrumbs. But I can't figure out what is the right one to put up there.

  2. We should probably move the doc id in the doc viewer page to the breadcrumbs and get rid of the tabs as there can never be more than one...(?)

@cchaos
Copy link
Contributor

cchaos commented Dec 4, 2018

Maybe the breadcrumbs should be:

Discover / Context of _doc#p7SkdWcBMSKbnGD8wwQB

for context, and

Discover / _doc#p7SkdWcBMSKbnGD8wwQB

for single doc viewer, and get rid of that extra title and "Surrounding Documents..." and the tabs.

But then do we think it's important to show the index pattern name somewhere?

@spalger
Copy link
Contributor Author

spalger commented Dec 4, 2018

I think the index pattern is more important than the type (_doc) since types are deprecated/removed in 7.0 anyway.

Avoiding truncation of the last breadcrumb based on available space would ease my worries, so let's go for that!

@spalger
Copy link
Contributor Author

spalger commented Dec 4, 2018

image

image

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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.

I think that makes sense to me!

@spalger spalger merged commit b4721ab into elastic:master Dec 4, 2018
@spalger spalger deleted the implement/k7-breadcrumbs/discover branch August 18, 2020 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants