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.

Sign up for GitHub

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

feat(menu): allow overriding home item in breadcrumbs #2848

Merged
merged 2 commits into from
Mar 19, 2024
Merged

feat(menu): allow overriding home item in breadcrumbs #2848

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7a1ca53
feat(menu): allow overriding home item in breadcrumbs
vicgeralds Mar 14, 2024
feb9fa8
Merge branch 'main' into menu-home-breadcrumb
vicgeralds Mar 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions etc/lime-elements.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,7 @@ export namespace Components {
"loading": boolean;
"open": boolean;
"openDirection": OpenDirection;
"rootItem": BreadcrumbsItem;
"searcher": MenuSearcher;
"surfaceWidth": SurfaceWidth;
}
Expand Down Expand Up @@ -1351,6 +1352,7 @@ namespace JSX_2 {
"onSelect"?: (event: LimelMenuCustomEvent<MenuItem>) => void;
"open"?: boolean;
"openDirection"?: OpenDirection;
"rootItem"?: BreadcrumbsItem;
"searcher"?: MenuSearcher;
"surfaceWidth"?: SurfaceWidth;
}
Expand Down
43 changes: 26 additions & 17 deletions src/components/menu/menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,17 @@ import {
} from '../../util/keycodes';

interface MenuCrumbItem extends BreadcrumbsItem {
menuItem: MenuItem;
menuItem?: MenuItem;
}

const DEFAULT_ROOT_BREADCRUMBS_ITEM: BreadcrumbsItem = {
text: '',
icon: {
name: 'home',
},
type: 'icon-only',
};

/**
* @slot trigger - Element to use as a trigger for the menu.
* @exampleComponent limel-example-menu-basic
Expand Down Expand Up @@ -136,6 +144,13 @@ export class Menu {
@Prop({ mutable: true })
public currentSubMenu: MenuItem;

/**
* A root breadcrumb item to show above the menu items.
* Clicking it navigates back from a sub-menu to the root menu.
*/
@Prop()
public rootItem: BreadcrumbsItem = DEFAULT_ROOT_BREADCRUMBS_ITEM;

/**
* Is emitted when the menu is cancelled.
*/
Expand Down Expand Up @@ -177,9 +192,6 @@ export class Menu {
@State()
private loadingSubItems: boolean;

@State()
private menuBreadCrumb: MenuCrumbItem[] = [];

@State()
private searchValue: string;

Expand Down Expand Up @@ -262,8 +274,7 @@ export class Menu {
}
}

@Watch('currentSubMenu')
protected currentSubMenuWatcher() {
private getBreadcrumbsItems() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this still reflect the content of the menu now that we're not longer watching for changes and re-rendering the breadcrumbs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const breadCrumbItems: MenuCrumbItem[] = [];
let currentItem = this.currentSubMenu;
while (currentItem) {
Expand All @@ -275,17 +286,14 @@ export class Menu {
currentItem = currentItem.parentItem;
}

if (breadCrumbItems.length) {
breadCrumbItems.push({
text: '',
icon: {
name: 'home',
},
type: 'icon-only',
} as MenuCrumbItem);
if (
breadCrumbItems.length ||
this.rootItem !== DEFAULT_ROOT_BREADCRUMBS_ITEM
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@vicgeralds vicgeralds Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

) {
breadCrumbItems.push(this.rootItem);
}

this.menuBreadCrumb = breadCrumbItems.reverse();
return breadCrumbItems.reverse();
}

private renderLoader = () => {
Expand All @@ -311,7 +319,8 @@ export class Menu {
};

private renderBreadcrumb = () => {
if (!this.menuBreadCrumb?.length) {
const breadcrumbsItems = this.getBreadcrumbsItems();
if (!breadcrumbsItems.length) {
return;
}

Expand All @@ -322,7 +331,7 @@ export class Menu {
'flex-shrink': '0',
}}
onSelect={this.handleBreadcrumbsSelect}
items={this.menuBreadCrumb}
items={breadcrumbsItems}
/>
);
};
Expand Down
Loading