feat(menu): allow overriding home item in breadcrumbs #2848
Conversation
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-2848/ |
vicgeralds
force-pushed
the
menu-home-breadcrumb
branch
from
f516491 to
7a1ca53
Compare
| } as MenuCrumbItem); | ||
| if ( | ||
| breadCrumbItems.length || | ||
| this.rootItem !== DEFAULT_ROOT_BREADCRUMBS_ITEM |
There was a problem hiding this comment.
This means that you'd have to add a custom root item even though you might just want to show the default? Should we make it possible to just choose to show the root item?
There was a problem hiding this comment.
Yes, but I think it's not very good to just show the default. It has no text and shows an empty tooltip. Showing a generic icon that works well everywhere is difficult, and default text is difficult because it needs to be translated.
So to use breadcrumbs, I think this needs to be provided.
There was a problem hiding this comment.
I'm a little unsure how to interpret the above. It sounds like you are describing a breaking change, but since the examples are still working as expected, and you haven't had to update any of them in this PR, I assume it's not actually a breaking change? 😅
There was a problem hiding this comment.
It’s not a breaking change. I’m just complaining a bit about the default home item that’s rendered with an empty tooltip text. It’s better than nothing if you’re showing a sub menu, but I think you’d always want to replace it by providing rootItem.
There is a slight inconsistency introduced by always showing the breadcrumbs when rootItem is provided, not just after navigating to a submenu. I think this is better, because it means the position of the menu items doesn’t jump up/down when navigating between submenus and root menu.
| @@ -262,8 +274,7 @@ export class Menu { | |||
| } | |||
| } | |||
|
|
|||
| @Watch('currentSubMenu') | |||
| protected currentSubMenuWatcher() { | |||
| private getBreadcrumbsItems() { | |||
There was a problem hiding this comment.
Will this still reflect the content of the menu now that we're not longer watching for changes and re-rendering the breadcrumbs?
There was a problem hiding this comment.
Yes, because currentSubMenu is a (mutable) prop. So we will always re-render when it changes, and we will always call this.getBreadcrumbsItems() when rendering. It simplifies things to remove the watcher, and it also means breadcrumbs are rendered on first render, rather than only when currentSubMenu changes.
|
🎉 This PR is included in version 37.12.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Adds a new property
rootItemtolimel-menufor specifying the first item when showing breadcrumbs.If specified, the breadcrumbs will always be shown. Otherwise, the breadcrumbs are only shown when a sub-menu is open.
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: