[EuiBreadcrumbs] Always show collapsed items & Amsterdam update to [EuiHeaderBreadcrumbs] #3578

cchaos · 2020-06-08T20:29:51Z

Part 3/3 for next iteration of K8 support (Amsterdam breadcrumbs)

Digging into the styles and render of EuiBreadcrumbs led me down the path add some enhancements/changes to the component itself.

1. Always show collapsed items when `max` is set

I removed the old showMaxPopover prop and instead made this the default render method of the breadcrumbs. It's a much better design to allow users to always be able to navigate all breadcrumbs at any time.

It also changes the display of the collapsed item slightly to simply show a down arrow next to the ellipsis.

Before

After

2. Responsive breadcrumbs always show collapsed items

Before, the responsive nature of breadcrumbs would hide the display of items with CSS as the window width got smaller. Again this meant that the user could never view or get to the earlier breadcrumbs in the list which could then become mostly unhelpful for them.

Now, I'm adjusting max property depending on the width of the browser and the current breakpoint (if responsive is true of course).

Which also lead me to need the creation of a global responsive/breakpoint service/helper so that it's also easy for consumers to override the specific breakpoints.

3. Amsterdam theme

Because we found usages of EuiBreadcrumbs outside of the header, for example in EuiControlBar, we decided to only update the EuiHeaderBreadcrumbs to the more stylized version for Amsterdam.

Other Code Considerations

A. I didn't consider the removal of showMaxPopover to be a breaking change since this behavior is always on now. It will just alert consumers that the prop no longer exists.
B. Renamed Breadcrumb prop export to EuiBreadcrumb (is this considered a break?)
C. Created a euiButtonDefaultStyle() mixin for Amsterdam's transparent background style buttons for reuse on other components like the header breadcrumbs.
D. This doesn't tackle any accessibility issues since it's already become such a large PR. We can do a follow-up. cc @myasonik

Breaking change:

Changed breadcrumb TS type exported name from Breadcrumb to EuiBreadcrumb

Checklist

Check against all themes for compatibility in both light and dark modes
Checked in mobile
Checked in Safari and Firefox
Props have proper autodocs
Added documentation
Checked Code Sandbox works for the any docs examples
Added or updated jest tests
Checked for breaking changes and labeled appropriately
~~[ ] Checked for accessibility including keyboard-only and screenreader modes~~
A changelog entry exists and is marked appropriately

…popover

- Added BREAKPOINTS and breakpoint types in `/services` - Add all props and fixup examples - Breadcrumb become EuiBreadcrumb - Fixes contrast for EuiControlBar’s usage (not perfect)

snide

Functional changes look fantastic 🎉

Before I jump too far into the review, the only thing that looked a bit odd to me visually was using the caret dropdown for the ... stuff. I preferred the original way where it was more buttonized so you didn't need the caret.

I'm guessing this change was made because amsterdam buttonizes all the breadcrumbs, so it was hard to make it look distinct. Could we potentially just make that button a little darker or something to call it out a little more. I'm just trying to be thoughtful towards the space. Only other alternative I have is to maybe use a more involved icon rather than the text + icon treatment we're using here?

kibanamachine · 2020-06-08T20:52:41Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3578/

cchaos · 2020-06-08T22:03:24Z

FYI, I decided I wanted to change how I'm declaring BREAKPOINTS and how EuiBreadcrumbs handles the custom responsiveness. Working on that now and finalizing the changes. Just a heads up :)

And then how EuiBreadcrumbs handle the custom responsive option

cchaos · 2020-06-08T22:52:56Z

@snide Yeah we talked about it quite a bit in Slack and ended up on this particular display because it:

Didn't call too much attention to itself. This doesn't need to be a super important since apps shouldn't rely solely on breadcrumbs for navigation
It works well when visually combined with truncated items like so:

Also it was quite cumbersome to try to get the new style to work on both links and badges and was easier to just make them all links.

cchaos · 2020-06-08T22:54:09Z

Also, I've pushed the changes that I talked about and this should be reviewable again. 🙇‍♀️

myasonik · 2020-06-08T23:11:13Z

Haven't reviewed this at all yet but I don't imagine there'll be many a11y concerns roughly knowing what was happening here before.

Do you want me to review and leave comments on this PR that you can then do whatever with or do you want me to open a new issue for any that come up?

kibanamachine · 2020-06-08T23:11:32Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3578/

chandlerprall

Few small items, overall the change look great

src/components/breadcrumbs/breadcrumbs.tsx

src/services/responsive.ts

cchaos · 2020-06-09T18:12:10Z

Do you want me to review and leave comments on this PR that you can then do whatever with or do you want me to open a new issue for any that come up?

@myasonik I'd like to push any a11y specific issues for a follow up PR, so yes maybe one meta issue for breadcrumb accessibility would be great. So no need to review this particular PR, but could be a starting to point to look at the overall accessibility of the component.

Co-authored-by: Chandler Prall <chandler.prall@gmail.com>

kibanamachine · 2020-06-09T18:39:46Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3578/

- Fixing some test suites - Fixing aria-current when no href - Making sure `max` overrides larger breakpoint values

cchaos · 2020-06-09T19:38:52Z

src-docs/src/views/breadcrumbs/props.tsx

+export const BreadcrumbResponsiveMaxCount: FunctionComponent<
+  EuiBreadcrumbResponsiveMaxCount
+> = () => <div />;


@chandlerprall I can't seem to get this one to show up in the props table

It's working for me - try stopping your dev server, rm -rf .cache-loader, and re-start the server. Alternatively, try changing BreadcrumbProps's div to a span - this is often enough to flush the webpack cache for a specific prop object.

snide

Might want to adjust the header breadcrumbs when not in amersterdam. Likely can just turn off the left padding for :first-child

Code looks good. We're getting pretty overwritey in amsterdam. I don't know a way around it, so I'm looking in another direction 😄

src/components/breadcrumbs/_breadcrumbs.scss

kibanamachine · 2020-06-09T20:02:33Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3578/

cchaos · 2020-06-09T20:14:34Z

Might want to adjust the header breadcrumbs when not in amersterdam. Likely can just turn off the left padding for :first-child

There is no padding in the non-Amsterdam version. What you're seeing is produced by the lack of divider, so I added the border in so you can see how it actually lines up.

kibanamachine · 2020-06-09T20:37:23Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3578/

chandlerprall

Changes LGTM!

snide

Thanks for the explanation and fixes. LGTM

cchaos added 4 commits June 5, 2020 15:57

[EuiBreadcrumbs] Changed collapsed badge from using EuiBadge to EuiLink

1b914fe

Remove showMaxPopover prop and always display collapsed items in a …

664bc92

…popover

[EuiBreadcrumbs] Fixup responsive to display collapsed

f76a85f

- Added BREAKPOINTS and breakpoint types in `/services` - Add all props and fixup examples - Breadcrumb become EuiBreadcrumb - Fixes contrast for EuiControlBar’s usage (not perfect)

Amsterdam style EuiHeaderBreadcrumbs

c7e3340

cchaos added the Amsterdam label Jun 8, 2020

cchaos requested review from snide and chandlerprall June 8, 2020 20:30

snide reviewed Jun 8, 2020

View reviewed changes

cchaos and others added 3 commits June 8, 2020 18:45

Changing how EuiBreakpoint works

6550eeb

And then how EuiBreadcrumbs handle the custom responsive option

cl

97a981c

Merge branch 'master' into k8/amsterdam/breadcrumbs

ed22aef

chandlerprall suggested changes Jun 9, 2020

View reviewed changes

Apply suggestions from code review

619d314

Co-authored-by: Chandler Prall <chandler.prall@gmail.com>

Updating responsive to accept true

e788106

- Fixing some test suites - Fixing aria-current when no href - Making sure `max` overrides larger breakpoint values

cchaos commented Jun 9, 2020

View reviewed changes

snide reviewed Jun 9, 2020

View reviewed changes

src/components/breadcrumbs/_breadcrumbs.scss Outdated Show resolved Hide resolved

cchaos added 3 commits June 9, 2020 16:06

Change responsive file name to breakpoint and add tests

07eba0e

Fix stacked example

9241407

Change em to px

bf9250c

Fix stacked example pt 2

0a6cba0

cchaos requested a review from chandlerprall June 9, 2020 20:17

cchaos requested a review from snide June 9, 2020 21:23

chandlerprall approved these changes Jun 9, 2020

View reviewed changes

snide approved these changes Jun 9, 2020

View reviewed changes

cchaos added the breaking change label Jun 10, 2020

cchaos merged commit 0b5d6d2 into elastic:master Jun 10, 2020

cchaos deleted the k8/amsterdam/breadcrumbs branch June 10, 2020 14:34

johnbarrierwilson mentioned this pull request Jun 10, 2020

[META] EUI Amsterdam Roadmap #2639

Closed

thompsongl mentioned this pull request Jun 29, 2020

Upgrade EUI to v26.3.1 elastic/kibana#70243

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiBreadcrumbs] Always show collapsed items & Amsterdam update to [EuiHeaderBreadcrumbs] #3578

[EuiBreadcrumbs] Always show collapsed items & Amsterdam update to [EuiHeaderBreadcrumbs] #3578

cchaos commented Jun 8, 2020 •

edited

Loading

snide left a comment

kibanamachine commented Jun 8, 2020

cchaos commented Jun 8, 2020

cchaos commented Jun 8, 2020 •

edited

Loading

cchaos commented Jun 8, 2020

myasonik commented Jun 8, 2020

kibanamachine commented Jun 8, 2020

chandlerprall left a comment

cchaos commented Jun 9, 2020

kibanamachine commented Jun 9, 2020

cchaos Jun 9, 2020

chandlerprall Jun 9, 2020

snide left a comment

kibanamachine commented Jun 9, 2020

cchaos commented Jun 9, 2020

kibanamachine commented Jun 9, 2020

chandlerprall left a comment

snide left a comment

[EuiBreadcrumbs] Always show collapsed items & Amsterdam update to [EuiHeaderBreadcrumbs] #3578

[EuiBreadcrumbs] Always show collapsed items & Amsterdam update to [EuiHeaderBreadcrumbs] #3578

Conversation

cchaos commented Jun 8, 2020 • edited Loading

Part 3/3 for next iteration of K8 support (Amsterdam breadcrumbs)

1. Always show collapsed items when max is set

2. Responsive breadcrumbs always show collapsed items

3. Amsterdam theme

Other Code Considerations

Breaking change:

Checklist

snide left a comment

Choose a reason for hiding this comment

kibanamachine commented Jun 8, 2020

cchaos commented Jun 8, 2020

cchaos commented Jun 8, 2020 • edited Loading

cchaos commented Jun 8, 2020

myasonik commented Jun 8, 2020

kibanamachine commented Jun 8, 2020

chandlerprall left a comment

Choose a reason for hiding this comment

cchaos commented Jun 9, 2020

kibanamachine commented Jun 9, 2020

cchaos Jun 9, 2020

Choose a reason for hiding this comment

chandlerprall Jun 9, 2020

Choose a reason for hiding this comment

snide left a comment

Choose a reason for hiding this comment

kibanamachine commented Jun 9, 2020

cchaos commented Jun 9, 2020

kibanamachine commented Jun 9, 2020

chandlerprall left a comment

Choose a reason for hiding this comment

snide left a comment

Choose a reason for hiding this comment

cchaos commented Jun 8, 2020 •

edited

Loading

1. Always show collapsed items when `max` is set

cchaos commented Jun 8, 2020 •

edited

Loading