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.

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

Collapse main menu options in a hamburger menu #489

Merged
merged 20 commits into from
Feb 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
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
3 changes: 2 additions & 1 deletion examples/example-dockpanel/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
|----------------------------------------------------------------------------*/
import { CommandRegistry } from '@lumino/commands';

import { Message } from '@lumino/messaging';
import { Message, MessageLoop } from '@lumino/messaging';

import {
BoxPanel,
Expand Down Expand Up @@ -446,6 +446,7 @@ function main(): void {
main.addWidget(dock);

window.onresize = () => {
MessageLoop.postMessage(bar, new Widget.ResizeMessage(-1, -1));
main.update();
};

Expand Down
204 changes: 186 additions & 18 deletions packages/widgets/src/menubar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import { getKeyboardLayout } from '@lumino/keyboard';

import { Message, MessageLoop } from '@lumino/messaging';

import { CommandRegistry } from '@lumino/commands';

import {
ElementARIAAttrs,
ElementDataset,
Expand Down Expand Up @@ -47,6 +49,10 @@ export class MenuBar extends Widget {
forceX: true,
forceY: true
};
this._overflowMenuOptions = options.overflowMenuOptions || {
overflowMenuVisible: true,
title: '...'
};
}

/**
Expand All @@ -73,6 +79,20 @@ export class MenuBar extends Widget {
return this._childMenu;
}

/**
* The overflow index of the menu bar.
*/
get overflowIndex(): number {
return this._overflowIndex;
}

/**
* The overflow menu of the menu bar.
*/
get overflowMenu(): Menu | null {
return this._overflowMenu;
}

/**
* Get the menu bar content node.
*
Expand Down Expand Up @@ -188,8 +208,8 @@ export class MenuBar extends Widget {
* #### Notes
* If the menu is already added to the menu bar, it will be moved.
*/
addMenu(menu: Menu): void {
this.insertMenu(this._menus.length, menu);
addMenu(menu: Menu, update: boolean = true): void {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @krassowski, I remembered that we added this optional argument here in Lumino just in case other applications didn't want to have the collapse behavior. I can check if this argument is set to True in Jupyter, and that should be the solution of not seeing this feature active

this.insertMenu(this._menus.length, menu, update);
}

/**
Expand All @@ -204,7 +224,7 @@ export class MenuBar extends Widget {
*
* If the menu is already added to the menu bar, it will be moved.
*/
insertMenu(index: number, menu: Menu): void {
insertMenu(index: number, menu: Menu, update: boolean = true): void {
// Close the child menu before making changes.
this._closeChildMenu();

Expand All @@ -228,7 +248,9 @@ export class MenuBar extends Widget {
menu.title.changed.connect(this._onTitleChanged, this);

// Schedule an update of the items.
this.update();
if (update) {
this.update();
}

// There is nothing more to do.
return;
Expand All @@ -250,7 +272,9 @@ export class MenuBar extends Widget {
ArrayExt.move(this._menus, i, j);

// Schedule an update of the items.
this.update();
if (update) {
this.update();
}
}

/**
Expand All @@ -261,8 +285,8 @@ export class MenuBar extends Widget {
* #### Notes
* This is a no-op if the menu is not in the menu bar.
*/
removeMenu(menu: Menu): void {
this.removeMenuAt(this._menus.indexOf(menu));
removeMenu(menu: Menu, update: boolean = true): void {
this.removeMenuAt(this._menus.indexOf(menu), update);
}

/**
Expand All @@ -273,7 +297,7 @@ export class MenuBar extends Widget {
* #### Notes
* This is a no-op if the index is out of range.
*/
removeMenuAt(index: number): void {
removeMenuAt(index: number, update: boolean = true): void {
// Close the child menu before making changes.
this._closeChildMenu();

Expand All @@ -294,7 +318,9 @@ export class MenuBar extends Widget {
menu.removeClass('lm-MenuBar-menu');

// Schedule an update of the items.
this.update();
if (update) {
this.update();
}
}

/**
Expand Down Expand Up @@ -387,6 +413,14 @@ export class MenuBar extends Widget {
}
}

/**
* A message handler invoked on a `'resize'` message.
*/
protected onResize(msg: Widget.ResizeMessage): void {
this.update();
fcollonval marked this conversation as resolved.
Show resolved Hide resolved
super.onResize(msg);
}

/**
* A message handler invoked on an `'update-request'` message.
*/
Expand All @@ -398,23 +432,128 @@ export class MenuBar extends Widget {
this._tabFocusIndex >= 0 && this._tabFocusIndex < menus.length
? this._tabFocusIndex
: 0;
let content = new Array<VirtualElement>(menus.length);
for (let i = 0, n = menus.length; i < n; ++i) {
let title = menus[i].title;
let active = i === activeIndex;
if (active && menus[i].items.length == 0) {
active = false;
}
let length = this._overflowIndex > -1 ? this._overflowIndex : menus.length;
let totalMenuSize = 0;
let overflowMenuVisible = false;

// Check that the overflow menu doesn't count
length = this._overflowMenu !== null ? length - 1 : length;
let content = new Array<VirtualElement>(length);

// Render visible menus
for (let i = 0; i < length; ++i) {
content[i] = renderer.renderItem({
title,
active,
title: menus[i].title,
active: i === activeIndex && menus[i].items.length !== 0,
tabbable: i === tabFocusIndex,
onfocus: () => {
this.activeIndex = i;
}
});
// Calculate size of current menu
totalMenuSize += this._menuItemSizes[i];
// Check if overflow menu is already rendered
if (menus[i].title.label === this._overflowMenuOptions.title) {
overflowMenuVisible = true;
length--;
}
}
// Render overflow menu if needed and active
if (this._overflowMenuOptions.overflowMenuVisible) {
if (this._overflowIndex > -1 && !overflowMenuVisible) {
// Create overflow menu
if (this._overflowMenu === null) {
this._overflowMenu = new Menu({ commands: new CommandRegistry() });
this._overflowMenu.title.label = this._overflowMenuOptions.title;
this._overflowMenu.title.mnemonic = 0;
this.addMenu(this._overflowMenu, false);
}
// Move menus to overflow menu
for (let i = menus.length - 2; i >= length; i--) {
const submenu = this.menus[i];
submenu.title.mnemonic = 0;
this._overflowMenu.insertItem(0, {
type: 'submenu',
submenu: submenu
});
this.removeMenu(submenu, false);
}
content[length] = renderer.renderItem({
title: this._overflowMenu.title,
active: length === activeIndex && menus[length].items.length !== 0,
tabbable: length === tabFocusIndex,
onfocus: () => {
this.activeIndex = length;
}
});
length++;
} else if (this._overflowMenu !== null) {
// Remove submenus from overflow menu
let overflowMenuItems = this._overflowMenu.items;
let screenSize = this.node.offsetWidth;
let n = this._overflowMenu.items.length;
for (let i = 0; i < n; ++i) {
let index = menus.length - 1 - i;
if (screenSize - totalMenuSize > this._menuItemSizes[index]) {
let menu = overflowMenuItems[0].submenu as Menu;
this._overflowMenu.removeItemAt(0);
this.insertMenu(length, menu, false);
content[length] = renderer.renderItem({
title: menu.title,
active: false,
tabbable: length === tabFocusIndex,
onfocus: () => {
this.activeIndex = length;
}
});
length++;
}
}
if (this._overflowMenu.items.length === 0) {
this.removeMenu(this._overflowMenu, false);
content.pop();
this._overflowMenu = null;
this._overflowIndex = -1;
}
}
}
VirtualDOM.render(content, this.contentNode);
this._updateOverflowIndex();
}

/**
* Calculate and update the current overflow index.
*/
private _updateOverflowIndex(): void {
// Get elements visible in the main menu bar
const itemMenus = this.contentNode.childNodes;
let screenSize = this.node.offsetWidth;
let totalMenuSize = 0;
let index = -1;
let n = itemMenus.length;

if (this._menuItemSizes.length == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

To reduce the number of lines in this resize handler, maybe you want to replace this._menuItemSizes with a _getMenuItemSizes method that handles this caching of the menu item sizes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can move this block of code to another function but I would need to send almost all the variables defined in the function as a parameter, so I don't know what's best...

Copy link
Contributor

Choose a reason for hiding this comment

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

It might just be me, but would it be better if the entire function were moved into a separate method? Like so:

_evtResize(event: Event): void {
  this._handleOverflow();
}

I'm not sure that's the best name for the method, but there are a few reasons why I think having such a method would be a good idea:

  • I think it might be prudent to provide a way for clients to opt out of this behavior. I don't know all of the places that the Lumino menu bar class is used. I'm not 100% sure this behavior should be forced on downstream consumers always. Pulling the body of this function into its own method will make it easy to isolate that code and exit early if the client has passed some kind of "no overflow" option to the menu bar.
  • Once we figure out how to hook into page load and where, we will be able to call this method (rather than this._evtResize) from there.
  • Slightly simplified unit testing, since you don't have to trigger a resize event (also it makes it clear that you're not actually using the event object)
  • Friendlier to future programmers if they want to add more stuff to the resize handler (that doesn't have to do with overflow)

// Check if it is the first resize and get info about menu items sizes
for (let i = 0; i < n; i++) {
let item = itemMenus[i] as HTMLLIElement;
// Add sizes to array
totalMenuSize += item.offsetWidth;
this._menuItemSizes.push(item.offsetWidth);
if (totalMenuSize > screenSize && index === -1) {
index = i;
}
}
} else {
github-advanced-security[bot] marked this conversation as resolved.
Fixed
Show resolved Hide resolved
// Calculate current menu size
for (let i = 0; i < this._menuItemSizes.length; i++) {
Fixed Show fixed Hide fixed
totalMenuSize += this._menuItemSizes[i];
if (totalMenuSize > screenSize) {
index = i;
break;
}
}
}
this._overflowIndex = index;
}

/**
Expand Down Expand Up @@ -741,8 +880,12 @@ export class MenuBar extends Widget {
// Track which item can be focused using the TAB key. Unlike _activeIndex will always point to a menuitem.
private _tabFocusIndex = 0;
private _forceItemsPosition: Menu.IOpenOptions;
private _overflowMenuOptions: IOverflowMenuOptions;
private _menus: Menu[] = [];
private _childMenu: Menu | null = null;
private _overflowMenu: Menu | null = null;
private _menuItemSizes: number[] = [];
private _overflowIndex: number = -1;
}

/**
Expand All @@ -769,6 +912,15 @@ export namespace MenuBar {
* The default is `true`.
*/
forceItemsPosition?: Menu.IOpenOptions;
/**
* Whether to add a overflow menu if there's overflow.
*
* Setting to `true` will enable the logic that creates an overflow menu
* to show the menu items that don't fit entirely on the screen.
*
* The default is `true`.
*/
overflowMenuOptions?: IOverflowMenuOptions;
}

/**
Expand Down Expand Up @@ -952,6 +1104,22 @@ export namespace MenuBar {
export const defaultRenderer = new Renderer();
}

/**
* Options for overflow menu.
*/
export interface IOverflowMenuOptions {
/**
* Determines if a overflow menu appears when the menu items overflow.
*/
overflowMenuVisible: boolean;
/**
* Determines the title of the overflow menu.
*
* Default: `...`.
*/
title: string;
}

/**
* The namespace for the module implementation details.
*/
Expand Down
28 changes: 28 additions & 0 deletions packages/widgets/tests/src/menubar.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -901,6 +901,34 @@ describe('@lumino/widgets', () => {
expect(child.className).to.contain('lm-MenuBar-item');
bar.dispose();
});

it('should render the overflow menu', () => {
let bar = createMenuBar();
expect(bar.overflowIndex).to.equal(-1);
expect(bar.overflowMenu).to.equal(null);
bar.node.style.maxWidth = '70px';
MessageLoop.sendMessage(bar, Widget.Msg.UpdateRequest);
requestAnimationFrame(() => {
expect(bar.overflowMenu).to.not.equal(null);
expect(bar.overflowIndex).to.not.equal(-1);
bar.dispose();
});
});

it('should hide the overflow menu', () => {
let bar = createMenuBar();
expect(bar.overflowIndex).to.equal(-1);
expect(bar.overflowMenu).to.equal(null);
bar.node.style.maxWidth = '70px';
MessageLoop.sendMessage(bar, Widget.Msg.UpdateRequest);
bar.node.style.maxWidth = '400px';
MessageLoop.sendMessage(bar, Widget.Msg.UpdateRequest);
requestAnimationFrame(() => {
expect(bar.overflowMenu).to.equal(null);
expect(bar.overflowIndex).to.equal(-1);
bar.dispose();
});
});
});

context('`menuRequested` signal', () => {
Expand Down
Loading