-
Notifications
You must be signed in to change notification settings - Fork 842
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
Add EuiBreadcrumbs. #815
Add EuiBreadcrumbs. #815
Conversation
🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to have a BC system that can be independent of the main header one, but can we make them visually look the same? Or preferably be an alternate size prop applied to the same component since they do the same thing? Seems weird to have two different components when there isn't much difference with what we're doing with them? I don't think we need two breadcrumb systems, just a different style (mostly padding) for an in page one.
This one will also run into the same problems in lower resolutions that the live Kibana one does. It'd be nice to have some safeguards in there re: truncation and such, even if that's just passed as a optional prop. Titles in our systems (I'm looking at you generated ID or node names) tend to be HUGE and it'll just fall apart with anything large in it.
Stylistically I'd want them to be the same. So use "/" or ">" for both of them.
What are we trying to do differently then the ones in the header? Looks to me like we want something without the padding and a smaller footprint?
@snide I talked this over a bit with @gjones. I'll update these breadcrumbs to use the same slash as EuiHeaderBreadcrumbs so we have a consistent separator. Good idea on a prop for truncation; I'll add that. Aside from the size and padding, these breadcrumbs also visually emphasize the current breadcrumb, whereas EuiHeaderBreadcrumbs emphasize the non-current breadcrumbs. I think these visual differences and the two different contexts in which these components will be used are good reasons to keep them separate components, though we can definitely find ways to share behavior and appearance moving forward. We could also rename this component to more clearly contrast against EuiHeaderBreadcrumbs if you want, e.g EuiPageBreadcrumbs? |
Let's just make them the same then. I'm not tied to the look of the header one if people want the current node focused. I care more about consistency of style and not maintaining two components. Optimally I'd say we just make Does that make sense? |
If you want, I can get the styling bits in you'd need for this component to make these work in the header, then you can handle the JS for the |
Sounds good! |
@snide I added @chandlerprall Could you please review this again? |
@snide I'm starting to think that instead of creating a |
@cjcenizal still LGTM; I'd argue for keeping the |
Hmm I'm not sure if I communicated my idea clearly. I was thinking EUI would still be responsible for defining all of the size values, it just wouldn't be something the consumer would be able to set. Something like this: const EuiHeaderBreadcrumbs = () => {
return <EuiBreadcrumbs className="euiHeaderBreadcrumbs" />;
}; And then the Sass could set padding, size, etc (made-up values in this example): .euiHeaderBreadcrumbs {
font-size: $euiFontSizeM; // larger font-size
.euiBreadcrumb {
margin-right: $euiSizeL; // increased space between crumbs
}
// etc...
} WDYT? |
Ah, gotcha. That makes sense to me. |
Sorry. Missed this in my email when I got accessibility tunnel vision on friday. I'll get back to this later today. The size stuff vs. classes seems fine to me I think. I think you're right we probably will only have it in a couple places, and that's a fine way to handle it. TY for making the functionality changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. Much better, thanks for the update. I'll handle the following in a separate PR:
- Migrate the header to use this.
- Fix some minor styling issues.
- Add a tooltip when truncate is used.
- Adjust responsive to be a little more smart about widths.
I think we might want to set some defaults for the new options. Thinking mostly towards plop in best experience for people. Responsive: true, maxItems:5?
} | ||
|
||
.euiBreadcrumb--last { | ||
font-weight: 500; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be a var. Open sans doesn't have a 500
weight, so we probably want to use $euiFontWeightMedium
@snide Thanks for the feedback. I made these changes:
|
And I just set |
👍 Sounds good. I'm in Kibana land for a couple days, but will migrate the header stuff over this week. |
Came up with this design with @gjones: