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

Collapse main menu options in a hamburger menu #489

Merged
merged 20 commits into from
Feb 23, 2023
Merged

Collapse main menu options in a hamburger menu #489

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
2f46ffc
Make menubar responsive
steff456 Dec 6, 2022
59291de
Add review changes
steff456 Dec 7, 2022
4f9f0e1
Use cached item sizes
steff456 Dec 13, 2022
dfe9abd
Invalidate cached item sizes in onUpdateRequest
steff456 Dec 13, 2022
b5402a8
Add review changes
steff456 Dec 16, 2022
f7501b7
Move logic to onUpdateRequest
steff456 Dec 29, 2022
1e1c494
Remove redundant initialization
steff456 Dec 29, 2022
de210ed
Initialize overflowIndex and add submenus in the correct order
steff456 Dec 29, 2022
4e8c625
Create interface in renderer and change updateOverflowIndex as a priv…
steff456 Jan 23, 2023
c14d733
Get elements by children nodes without their classname
steff456 Jan 23, 2023
fcc357b
Collapse logic to reduce code complexity
krassowski Jan 25, 2023
8cca0f6
Merge pull request #1 from krassowski/collapse-menu-review
steff456 Jan 31, 2023
aae3aca
Undo dedicated IRenderer and add overflowMenuOptions
steff456 Feb 1, 2023
36cefff
Fix double menu bug
steff456 Feb 8, 2023
92fbf60
Add test to check the hamburger menu is rendering
steff456 Feb 15, 2023
8598c45
Merge remote-tracking branch 'upstream/main' into collapse-menu
steff456 Feb 16, 2023
3f0c8e6
Test that the overflow menu renders and hides
steff456 Feb 16, 2023
76b3125
Update API documentation
afshin Feb 18, 2023
f1f3879
Update packages/widgets/src/menubar.ts
fcollonval Feb 22, 2023
705a855
Break test into two to avoid having two animation frames
steff456 Feb 22, 2023
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
View file
Open in desktop
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
143 changes: 132 additions & 11 deletions packages/widgets/src/menubar.ts
View file
Open in desktop
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 @@ -183,8 +185,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 @@ -199,7 +201,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 @@ -223,7 +225,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 @@ -245,7 +249,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 @@ -256,8 +262,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 @@ -268,7 +274,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 @@ -289,7 +295,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 @@ -382,15 +390,61 @@ 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
}

protected updateOverflowIndex(): void {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to expose this method, or could it be private? _overflowIndex and _menuItemSizes are private so overriding this method would be rather difficult as it stands

If it should be protected to allow overriding, maybe it could return a new (private) interface, like:

interface IMenuOverflowState {
   itemSizes: number[];
   index: number
}

then you can have a single private variable set as:

this._overflowState = this.calculateOverflowState();

and use it with unpacking:

if (!this._overflowState) {
   return; // or otherwise skip
}
const { index: overflowIndex, itemSizes: menuItemSizes } = this._overflowState;

Then it would be also easier to unit-test it too.

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 tried to create the interface, but soon enough I found myself wondering that I needed to update the overflow index value in multiple spots, so I'm not sure if we need the interface. For now I just converted the function to private.

// Get elements visible in the main menu bar
let itemMenus = this.node.getElementsByClassName('lm-MenuBar-item');
steff456 marked this conversation as resolved.
Show resolved Hide resolved
steff456 marked this conversation as resolved.
Show resolved Hide resolved
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;
}

/**
* A message handler invoked on an `'update-request'` message.
*/
protected onUpdateRequest(msg: Message): void {
let menus = this._menus;
let renderer = this.renderer;
let activeIndex = this._activeIndex;
let content = new Array<VirtualElement>(menus.length);
for (let i = 0, n = menus.length; i < n; ++i) {
let length = this._overflowIndex > -1 ? this._overflowIndex : menus.length;
let content = new Array<VirtualElement>(length);
let totalMenuSize = 0;

// Check that the overflow menu doesn't count
length = this._overflowMenu !== null ? length - 1 : length;

// Render visible menus
for (let i = 0; i < length; ++i) {
let title = menus[i].title;
let active = i === activeIndex;
if (active && menus[i].items.length == 0) {
Expand All @@ -403,8 +457,72 @@ export class MenuBar extends Widget {
this.activeIndex = i;
}
});
// Calculate size of current menu
totalMenuSize += this._menuItemSizes[i];
}
// Render overflow menu if needed
if (this._overflowIndex > -1) {
// Create overflow menu
if (this._overflowMenu === null) {
this._overflowMenu = new Menu({ commands: new CommandRegistry() });
this._overflowMenu.title.label = '...';
this._overflowMenu.title.mnemonic = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe worth adding a new optional method in IRenderer, e.g. createOverflowMenu() that would allow to customise this? In particular downstream may want to customize label. You could provide a default implementation in Renderer.

Copy link
Member

Choose a reason for hiding this comment

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

I see that 4e8c625 added renderOverflowItem which is a copy-paste of renderItem and is used to replace it; I think that we should . This might be useful (though I would avoid copy-pasting the code and instead just invoke the other method by default), but this is not what I had in mind.

I meant to suggest that we should wrap rendering of the overflow menu so that the .... and mnemonic can be customized downstream. The lines that I commented on would then become for example:

Suggested change
this._overflowMenu.title.label = '...';
this._overflowMenu.title.mnemonic = 0;
this.renderer.decorateOverflowMenu(this._overflowMenu.title)

This is very minor, I don't think we have to do this. To limit the surface of API changes I would also suggest reverting renderOverflowItem for now - if this is needed in the future we can always add it (removing things is more difficult).

Copy link
Member

Choose a reason for hiding this comment

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

I think @krassowski's suggestion to remove renderOverflowItem until it turns out it is necessary is a good plan.

this.addMenu(this._overflowMenu, false);
}
// Move menus to overflow menu
for (let i = menus.length - 2; i >= length; i--) {
let submenu = this.menus[i];
steff456 marked this conversation as resolved.
Show resolved Hide resolved
submenu.title.mnemonic = 0;
this._overflowMenu.insertItem(0, {
type: 'submenu',
submenu: submenu
});
this.removeMenu(submenu, false);
}
let title = this._overflowMenu.title;
let active = length === activeIndex;
if (active && menus[length].items.length == 0) {
active = false;
}
content[length] = renderer.renderItem({
title,
active,
steff456 marked this conversation as resolved.
Show resolved Hide resolved
onfocus: () => {
this.activeIndex = 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);
let title = menu.title;
let active = false;
content[length] = renderer.renderItem({
title,
active,
steff456 marked this conversation as resolved.
Show resolved Hide resolved
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();
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this one will lead to the state being always delayed by one tick. It is not bad in itself if this is a conscious decision made as a performance-UX tradeoff. Could we document this?

One thing to consider is whether we should wrap it in an extra requestAnimationFrame to protect against layout trashing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do I wrap it in a requestAnimationFrame?

Copy link
Member

Choose a reason for hiding this comment

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

It would be something like:

requestAnimationFrame(() => this._updateOverflowIndex());

I have not tested if this helps. You would need to record a profile in developer tools when you resize the page to trigger collapsing before and after applying the change and compare. If you have the profiles it should be obvious from the time spend on forced style recalculation/layout whether it is worth waiting for the next frame or not.

}

/**
Expand Down Expand Up @@ -730,6 +848,9 @@ export class MenuBar extends Widget {
private _forceItemsPosition: Menu.IOpenOptions;
private _menus: Menu[] = [];
private _childMenu: Menu | null = null;
private _overflowMenu: Menu | null = null;
private _menuItemSizes: number[] = [];
private _overflowIndex: number = -1;
}

/**
Expand Down