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

Changes from 2 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
76 changes: 76 additions & 0 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 @@ -47,6 +49,8 @@ export class MenuBar extends Widget {
forceX: true,
forceY: true
};
this._hamburgerMenu = null;
steff456 marked this conversation as resolved.
Show resolved Hide resolved
this._menuItemSizes = [];
}

/**
Expand Down Expand Up @@ -347,6 +351,9 @@ export class MenuBar extends Widget {
event.preventDefault();
event.stopPropagation();
break;
case 'resize':
this._evtResize(event);
break;
}
}

Expand All @@ -359,6 +366,7 @@ export class MenuBar extends Widget {
this.node.addEventListener('mousemove', this);
this.node.addEventListener('mouseleave', this);
this.node.addEventListener('contextmenu', this);
window.addEventListener('resize', this);
steff456 marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand All @@ -370,6 +378,7 @@ export class MenuBar extends Widget {
this.node.removeEventListener('mousemove', this);
this.node.removeEventListener('mouseleave', this);
this.node.removeEventListener('contextmenu', this);
window.removeEventListener('resize', this);
this._closeChildMenu();
}

Expand All @@ -382,6 +391,71 @@ export class MenuBar extends Widget {
}
}

/**
* A message handler invoked on an `'resize'` message.
steff456 marked this conversation as resolved.
Show resolved Hide resolved
*/
protected _evtResize(event: Event): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a resize event on initial page load? In other words, if the user makes their browser really skinny first, and then loads JupyterLab (and then never resizes their browser again), will this code get executed and put stuff into the "..." menu or will the user have to first resize their browser window?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, there's not a resize event on the initial page load, so users will first have to resize their window. I'm not sure if there's a signal that exists when the page is loaded that I can connect this method to. If you know of it please let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

@afshin @fcollonval is there a good signal for initial load?

This seems bad from an end user's perspective, doesn't it? For example, if I've set my browser to a higher zoom and then load the JupyterLab UI, as an end user I don't want to have to jiggle the window resize handle to get the menu bar to render properly.

That said, just to be clear I'm not asking us to block on this, because it's something that can be added later.

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;
let first = false;

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
first = true;
}

for (let i = 0; i < n; i++) {
let item = itemMenus[i] as HTMLLIElement;
totalMenuSize += item.offsetWidth;
steff456 marked this conversation as resolved.
Show resolved Hide resolved
if (first) {
// Add sizes to array
this._menuItemSizes.push(item.offsetWidth);
}
if (totalMenuSize > screenSize) {
index = i;
break;
}
}
if (first) {
first = false;
}
steff456 marked this conversation as resolved.
Show resolved Hide resolved
if (index > -1) {
// Create hamburger menu
if (this._hamburgerMenu === null) {
this._hamburgerMenu = new Menu({ commands: new CommandRegistry() });
Fixed Show fixed Hide fixed
this._hamburgerMenu.title.label = '...';
this._hamburgerMenu.title.mnemonic = 0;
steff456 marked this conversation as resolved.
Show resolved Hide resolved
this.addMenu(this._hamburgerMenu);
}

// Move menus
for (let i = index; i < n - 1; i++) {
let submenu = this.menus[i];
submenu.title.mnemonic = 0;
this._hamburgerMenu.insertItem(0, {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you iterate forward through the clipped menu items and on each iteration insert them into the overflow menu at 0, doesn't that end up putting them in backwards?

For example, imagine we have three menu items, File, Edit, View, and that the last two, Edit and View, both overflow the screen width. On the first loop run, this code will insert Edit at 0. On the second run, it will insert View at 0. Is that correct? Does that end up putting View above Edit in the "..." menu?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the functionality that I envisioned is that the order of the overflow menu is for the menu items that are left to right to gather top to bottom. Then, it is easier for me to preserve the order when unpacking the elements to the top menu again.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see now why the code doesn't behave the way I thought it would. The resize event fires so many times when resizing the window that it's hard to actually have this loop handle more than one item at a time and when the user shrinks the window, it's like moving backwards through the menu items (because as the window shrinks the menu items get popped off from right to left).

type: 'submenu',
submenu: submenu
});
this.removeMenuAt(i);
}
} else if (this._hamburgerMenu !== null) {
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 lost with this else-if block. So if we get here, it means index is -1, right? And that means that all of the menu items fit within the screen size, right? Does that include the "..." menu item?

I guess I'm not sure why there isn't a loop here that just pops all of the menu items from the overflow back into the menu bar, something like:

while (this._hamburgerMenu.items.length) {
  let menu = this._hamburgerMenu.items[0].submenu as Menu;
  this._hamburgerMenu.removeItemAt(0);
  this.insertMenu(i, menu);
}
this.removeMenu(this._hamburgerMenu);
this._hamburgerMenu = null;

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 is an else-if block because I don't want to unpack all the elements of the overflow menu if there's no space for them, and obviously I don't want to try to do it if the overflow menu doesn't exist in the first place.

One of my first implementations tried to unpack everything and then just calculate which elements needed to go to the overflow menu but it had a lot of issues when testing it because the top menu was rendering faster, so sometimes menus will get lost or they will be inserted in the wrong order. That's why I decided to just unpack one element at the time per resize event. At the end even if the user drags the window continuously, that's firing multiple resize events.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the best solution here is to debounce the code that handles overflow.

On the one hand, it doesn't seem hygienic to me to deliberately rely on the resize event firing multiple times as the user grows or shrinks the window (e.g., what if the user uses some kind of assistive tech to grow/shrink the window by a large x-delta all at once, firing only one resize event). But I also understand that this code becomes nearly impossible to manage if a resize event fires while you're still in the middle of readjusting the menu from the previous resize event, so maybe it ultimately needs a debounce.

let i = n - 1;
let hamburgerMenuItems = this._hamburgerMenu.items;
if (screenSize - totalMenuSize > this._menuItemSizes[i]) {
let menu = hamburgerMenuItems[0].submenu as Menu;
this._hamburgerMenu.removeItemAt(0);
this.insertMenu(i, menu);
}
if (this._hamburgerMenu.items.length === 0) {
this.removeMenu(this._hamburgerMenu);
this._hamburgerMenu = null;
}
}
}

/**
* A message handler invoked on an `'update-request'` message.
*/
Expand Down Expand Up @@ -730,6 +804,8 @@ export class MenuBar extends Widget {
private _forceItemsPosition: Menu.IOpenOptions;
private _menus: Menu[] = [];
private _childMenu: Menu | null = null;
private _hamburgerMenu: Menu | null = null;
private _menuItemSizes: number[] = [];
}

/**
Expand Down