Show all breadcrumbs in a popover #2342
Conversation
|
jenkins test this |
|
Tried this out quick, its a nice enhancement to navigation! A couple of quick observations regarding keyboard navigation...
🤔 I wonder if this should be the default behavior of the breadcrumb component? |
|
Furthering @ryankeairns' comments is that there's also no visual difference between the ellipsis that's clickable the one that is not. As a user, I could get used to the clickable version then think the other one is broken. Or get used to the non-clickable version and never know that there is a clickable version. I'm not sure what a good solution is, however. I think making it our primary color may stand out too much. It could just be the text color instead of grayed out, but it that enough. Maybe for now it is enough since even the regular clickable breadcrumbs are text colored. Or do we have a breadcrumb coloring problem in general? (Which doesn't need to be solved in this PR)/ |
|
@daveyholler Is this ready for review? |
|
@cchaos It is :) |
|
Quick design question while I review the code. Do you think it's expected that when clicking the "middle" truncated area, that the popover would show the full list instead of just those that are missing? I'm honestly on the fence. But just gauging if others felt some awkwardness with that. |
|
@cchaos I thought about that too, and concluded that the extra two items (1 on even smaller screens) was a small tradeoff for the full context. 🤷♂ |
That's only if the consumer provided a max of 3, what if the max was 10 and there were 20 items? Again, not advocation one way or the other, but we just need to think about all scenarios. |
Co-Authored-By: Caroline Horn <549577+cchaos@users.noreply.github.com>
…nto responsive-breadcrumbs
There was a problem hiding this comment.
LGTM, with one small nit and with @chandlerprall's approval
| // array so that all we have left are the items that aren't | ||
| // visible on screen. Only do it when showMaxPopover is true | ||
| if (showMaxPopover) { | ||
| for (let i = breadcrumbsToRemove.length - 1; i >= 0; i--) { |
There was a problem hiding this comment.
My head hurts. 🤕 @chandlerprall would you mind glancing over this logic. It does work, I'm just wondering if there's a more compact way to handle it?
Co-Authored-By: Caroline Horn <549577+cchaos@users.noreply.github.com>
Summary
Allows all breadcrumbs to be displayed in a popover. Useful when the list is truncated, but you still want users to be able to navigate to anywhere in the breadcrumbs list.
Checklist