From c372e4624c1bd3ff22e815fee093f0e9258e2139 Mon Sep 17 00:00:00 2001 From: Jeremy Tuloup Date: Sat, 4 Mar 2023 08:09:58 +0000 Subject: [PATCH 1/6] Make `IOverflowMenuOptions.title` optional --- packages/widgets/src/menubar.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/widgets/src/menubar.ts b/packages/widgets/src/menubar.ts index 5a45743cf..138cf7a50 100644 --- a/packages/widgets/src/menubar.ts +++ b/packages/widgets/src/menubar.ts @@ -51,7 +51,6 @@ export class MenuBar extends Widget { }; this._overflowMenuOptions = options.overflowMenuOptions || { overflowMenuVisible: true, - title: '...' }; } @@ -463,8 +462,9 @@ export class MenuBar extends Widget { if (this._overflowIndex > -1 && !overflowMenuVisible) { // Create overflow menu if (this._overflowMenu === null) { + const overflowMenuTitle = this._overflowMenuOptions.title ?? '...'; this._overflowMenu = new Menu({ commands: new CommandRegistry() }); - this._overflowMenu.title.label = this._overflowMenuOptions.title; + this._overflowMenu.title.label = overflowMenuTitle; this._overflowMenu.title.mnemonic = 0; this.addMenu(this._overflowMenu, false); } @@ -1110,6 +1110,8 @@ export namespace MenuBar { export interface IOverflowMenuOptions { /** * Determines if a overflow menu appears when the menu items overflow. + * + * Defaults to `true`. */ overflowMenuVisible: boolean; /** @@ -1117,7 +1119,7 @@ export interface IOverflowMenuOptions { * * Default: `...`. */ - title: string; + title?: string; } /** From beff752b66949c1d215bcf01030961bd7ba1c2cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Collonval?= Date: Mon, 6 Mar 2023 09:08:44 +0100 Subject: [PATCH 2/6] Lint code and add test --- packages/widgets/src/menubar.ts | 4 +-- packages/widgets/tests/src/menubar.spec.ts | 29 ++++++++++++++++++++-- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/packages/widgets/src/menubar.ts b/packages/widgets/src/menubar.ts index 138cf7a50..4806fcda4 100644 --- a/packages/widgets/src/menubar.ts +++ b/packages/widgets/src/menubar.ts @@ -50,7 +50,7 @@ export class MenuBar extends Widget { forceY: true }; this._overflowMenuOptions = options.overflowMenuOptions || { - overflowMenuVisible: true, + overflowMenuVisible: true }; } @@ -1110,7 +1110,7 @@ export namespace MenuBar { export interface IOverflowMenuOptions { /** * Determines if a overflow menu appears when the menu items overflow. - * + * * Defaults to `true`. */ overflowMenuVisible: boolean; diff --git a/packages/widgets/tests/src/menubar.spec.ts b/packages/widgets/tests/src/menubar.spec.ts index 69fbe56dc..41925a909 100644 --- a/packages/widgets/tests/src/menubar.spec.ts +++ b/packages/widgets/tests/src/menubar.spec.ts @@ -57,8 +57,8 @@ describe('@lumino/widgets', () => { let commands: CommandRegistry; - function createMenuBar(): MenuBar { - let bar = new MenuBar(); + function createMenuBar(options?: MenuBar.IOptions): MenuBar { + let bar = new MenuBar(options); for (let i = 0; i < 3; i++) { let menu = new Menu({ commands }); let item = menu.addItem({ command: DEFAULT_CMD }); @@ -132,6 +132,16 @@ describe('@lumino/widgets', () => { bar.dispose(); }); + it('should accept only overflowMenuVisible option', () => { + const bar = new MenuBar({ + overflowMenuOptions: { overflowMenuVisible: false } + }); + + expect(bar).to.be.an.instanceOf(MenuBar); + + bar.dispose(); + }); + it('should add the `lm-MenuBar` class', () => { let bar = new MenuBar(); expect(bar.hasClass('lm-MenuBar')).to.equal(true); @@ -929,6 +939,21 @@ describe('@lumino/widgets', () => { bar.dispose(); }); }); + + it('should not use the overflow menu', () => { + let bar = createMenuBar({ + overflowMenuOptions: { overflowMenuVisible: false } + }); + 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.overflowIndex).to.equal(-1); + expect(bar.overflowMenu).to.equal(null); + bar.dispose(); + }); + }); }); context('`menuRequested` signal', () => { From 95b4cc978635c65abfc49b4cb35a41fc5289c064 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Collonval?= Date: Mon, 6 Mar 2023 09:17:46 +0100 Subject: [PATCH 3/6] Fix API --- review/api/widgets.api.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/review/api/widgets.api.md b/review/api/widgets.api.md index faa6b85b8..16c8c9cf3 100644 --- a/review/api/widgets.api.md +++ b/review/api/widgets.api.md @@ -585,7 +585,7 @@ export namespace GridLayout { // @public export interface IOverflowMenuOptions { overflowMenuVisible: boolean; - title: string; + title?: string; } // @public From 16e8573febfecfedb4835f6e56b0c9bce660dc28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Collonval?= Date: Mon, 6 Mar 2023 11:17:52 +0100 Subject: [PATCH 4/6] Fix turning off menu overflow --- packages/widgets/src/menubar.ts | 4 + packages/widgets/tests/src/menubar.spec.ts | 40 +++--- .../widgets/tests/src/splitlayout.spec.ts | 114 +++++++----------- 3 files changed, 65 insertions(+), 93 deletions(-) diff --git a/packages/widgets/src/menubar.ts b/packages/widgets/src/menubar.ts index 4806fcda4..0d9a28ee2 100644 --- a/packages/widgets/src/menubar.ts +++ b/packages/widgets/src/menubar.ts @@ -525,6 +525,10 @@ export class MenuBar extends Widget { * Calculate and update the current overflow index. */ private _updateOverflowIndex(): void { + if (!this._overflowMenuOptions.overflowMenuVisible) { + return; + } + // Get elements visible in the main menu bar const itemMenus = this.contentNode.childNodes; let screenSize = this.node.offsetWidth; diff --git a/packages/widgets/tests/src/menubar.spec.ts b/packages/widgets/tests/src/menubar.spec.ts index 41925a909..aa159c2aa 100644 --- a/packages/widgets/tests/src/menubar.spec.ts +++ b/packages/widgets/tests/src/menubar.spec.ts @@ -886,19 +886,16 @@ describe('@lumino/widgets', () => { }); describe('#onUpdateRequest()', () => { - it('should be called when the title of a menu changes', done => { + it('should be called when the title of a menu changes', () => { let bar = new LogMenuBar(); let menu = new Menu({ commands }); bar.addMenu(menu); bar.activeIndex = 0; - expect(bar.methods.indexOf('onUpdateRequest')).to.equal(-1); + expect(bar.methods).to.not.include('onUpdateRequest'); menu.title.label = 'foo'; - expect(bar.methods.indexOf('onUpdateRequest')).to.equal(-1); - requestAnimationFrame(() => { - expect(bar.methods.indexOf('onUpdateRequest')).to.not.equal(-1); - bar.dispose(); - done(); - }); + MessageLoop.flush(); + expect(bar.methods).to.include('onUpdateRequest'); + bar.dispose(); }); it('should render the content', () => { @@ -918,11 +915,10 @@ describe('@lumino/widgets', () => { 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(); - }); + MessageLoop.flush(); + expect(bar.overflowMenu).to.not.equal(null); + expect(bar.overflowIndex).to.not.equal(-1); + bar.dispose(); }); it('should hide the overflow menu', () => { @@ -933,11 +929,10 @@ describe('@lumino/widgets', () => { 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(); - }); + MessageLoop.flush(); + expect(bar.overflowMenu).to.equal(null); + expect(bar.overflowIndex).to.equal(-1); + bar.dispose(); }); it('should not use the overflow menu', () => { @@ -948,11 +943,10 @@ describe('@lumino/widgets', () => { expect(bar.overflowMenu).to.equal(null); bar.node.style.maxWidth = '70px'; MessageLoop.sendMessage(bar, Widget.Msg.UpdateRequest); - requestAnimationFrame(() => { - expect(bar.overflowIndex).to.equal(-1); - expect(bar.overflowMenu).to.equal(null); - bar.dispose(); - }); + MessageLoop.flush(); + expect(bar.overflowIndex).to.equal(-1); + expect(bar.overflowMenu).to.equal(null); + bar.dispose(); }); }); diff --git a/packages/widgets/tests/src/splitlayout.spec.ts b/packages/widgets/tests/src/splitlayout.spec.ts index 302e6c4b6..0693514be 100644 --- a/packages/widgets/tests/src/splitlayout.spec.ts +++ b/packages/widgets/tests/src/splitlayout.spec.ts @@ -131,26 +131,22 @@ describe('@lumino/widgets', () => { ); }); - it('should post a fit request to the parent widget', done => { + it('should post a fit request to the parent widget', () => { let layout = new LogSplitLayout({ renderer }); let parent = new Widget(); parent.layout = layout; layout.orientation = 'vertical'; - requestAnimationFrame(() => { - expect(layout.methods).to.contain('onFitRequest'); - done(); - }); + MessageLoop.flush(); + expect(layout.methods).to.contain('onFitRequest', 'unexpected failure'); }); - it('should be a no-op if the value does not change', done => { + it('should be a no-op if the value does not change', () => { let layout = new LogSplitLayout({ renderer }); let parent = new Widget(); parent.layout = layout; layout.orientation = 'horizontal'; - requestAnimationFrame(() => { - expect(layout.methods).to.not.contain('onFitRequest'); - done(); - }); + MessageLoop.flush(); + expect(layout.methods).to.not.contain('onFitRequest'); }); }); @@ -166,26 +162,22 @@ describe('@lumino/widgets', () => { expect(layout.spacing).to.equal(10); }); - it('should post a fit rquest to the parent widget', done => { + it('should post a fit request to the parent widget', () => { let layout = new LogSplitLayout({ renderer }); let parent = new Widget(); parent.layout = layout; layout.spacing = 10; - requestAnimationFrame(() => { - expect(layout.methods).to.contain('onFitRequest'); - done(); - }); + MessageLoop.flush(); + expect(layout.methods).to.contain('onFitRequest'); }); - it('should be a no-op if the value does not change', done => { + it('should be a no-op if the value does not change', () => { let layout = new LogSplitLayout({ renderer }); let parent = new Widget(); parent.layout = layout; layout.spacing = 4; - requestAnimationFrame(() => { - expect(layout.methods).to.not.contain('onFitRequest'); - done(); - }); + MessageLoop.flush(); + expect(layout.methods).to.not.contain('onFitRequest'); }); }); @@ -253,7 +245,7 @@ describe('@lumino/widgets', () => { }); describe('#moveHandle()', () => { - it('should set the offset position of a split handle', done => { + it('should set the offset position of a split handle', () => { let parent = new Widget(); let layout = new SplitLayout({ renderer }); let widgets = [new Widget(), new Widget(), new Widget()]; @@ -272,10 +264,8 @@ describe('@lumino/widgets', () => { let handle = layout.handles[1]; let left = handle.offsetLeft; layout.moveHandle(1, left + 20); - requestAnimationFrame(() => { - expect(handle.offsetLeft).to.not.equal(left); - done(); - }); + MessageLoop.flush(); + expect(handle.offsetLeft).to.not.equal(left); }); }); @@ -330,17 +320,15 @@ describe('@lumino/widgets', () => { expect(hook.messages).to.contain('after-attach'); }); - it('should post a layout request for the parent widget', done => { + it('should post a layout request for the parent widget', () => { let layout = new LogSplitLayout({ renderer }); let parent = new Widget(); parent.layout = layout; let widget = new Widget(); Widget.attach(parent, document.body); layout.addWidget(widget); - requestAnimationFrame(() => { - expect(layout.methods).to.contain('onFitRequest'); - done(); - }); + MessageLoop.flush(); + expect(layout.methods).to.contain('onFitRequest'); }); }); @@ -361,7 +349,7 @@ describe('@lumino/widgets', () => { expect(layout.widgets[2]).to.equal(widget); }); - it('should post a a layout request to the parent', done => { + it('should post a a layout request to the parent', () => { let layout = new LogSplitLayout({ renderer }); let widgets = [new Widget(), new Widget(), new Widget()]; let parent = new Widget(); @@ -371,10 +359,8 @@ describe('@lumino/widgets', () => { }); let widget = widgets[0]; layout.insertWidget(2, widget); - requestAnimationFrame(() => { - expect(layout.methods).to.contain('onFitRequest'); - done(); - }); + MessageLoop.flush(); + expect(layout.methods).to.contain('onFitRequest'); }); }); @@ -407,7 +393,7 @@ describe('@lumino/widgets', () => { parent.dispose(); }); - it('should post a a layout request to the parent', done => { + it('should post a a layout request to the parent', () => { let layout = new LogSplitLayout({ renderer }); let widget = new Widget(); let parent = new Widget(); @@ -415,48 +401,42 @@ describe('@lumino/widgets', () => { layout.addWidget(widget); Widget.attach(parent, document.body); layout.removeWidget(widget); - requestAnimationFrame(() => { - expect(layout.methods).to.contain('onFitRequest'); - parent.dispose(); - done(); - }); + MessageLoop.flush(); + expect(layout.methods).to.contain('onFitRequest'); + parent.dispose(); }); }); describe('#onAfterShow()', () => { - it('should post an update to the parent', done => { + it('should post an update to the parent', () => { let layout = new LogSplitLayout({ renderer }); let parent = new Widget(); parent.layout = layout; parent.hide(); Widget.attach(parent, document.body); parent.show(); + MessageLoop.flush(); expect(layout.methods).to.contain('onAfterShow'); - requestAnimationFrame(() => { - expect(layout.methods).to.contain('onUpdateRequest'); - parent.dispose(); - done(); - }); + expect(layout.methods).to.contain('onUpdateRequest'); + parent.dispose(); }); }); describe('#onAfterAttach()', () => { - it('should post a layout request to the parent', done => { + it('should post a layout request to the parent', () => { let layout = new LogSplitLayout({ renderer }); let parent = new Widget(); parent.layout = layout; Widget.attach(parent, document.body); + MessageLoop.flush(); expect(layout.methods).to.contain('onAfterAttach'); - requestAnimationFrame(() => { - expect(layout.methods).to.contain('onFitRequest'); - parent.dispose(); - done(); - }); + expect(layout.methods).to.contain('onFitRequest'); + parent.dispose(); }); }); describe('#onChildShown()', () => { - it('should post a fit request to the parent', done => { + it('should post a fit request to the parent', () => { let parent = new Widget(); let layout = new LogSplitLayout({ renderer }); parent.layout = layout; @@ -467,17 +447,15 @@ describe('@lumino/widgets', () => { }); Widget.attach(parent, document.body); widgets[0].show(); + MessageLoop.flush(); expect(layout.methods).to.contain('onChildShown'); - requestAnimationFrame(() => { - expect(layout.methods).to.contain('onFitRequest'); - parent.dispose(); - done(); - }); + expect(layout.methods).to.contain('onFitRequest'); + parent.dispose(); }); }); describe('#onChildHidden()', () => { - it('should post a fit request to the parent', done => { + it('should post a fit request to the parent', () => { let parent = new Widget(); let layout = new LogSplitLayout({ renderer }); parent.layout = layout; @@ -487,12 +465,10 @@ describe('@lumino/widgets', () => { }); Widget.attach(parent, document.body); widgets[0].hide(); + MessageLoop.flush(); expect(layout.methods).to.contain('onChildHidden'); - requestAnimationFrame(() => { - expect(layout.methods).to.contain('onFitRequest'); - parent.dispose(); - done(); - }); + expect(layout.methods).to.contain('onFitRequest'); + parent.dispose(); }); }); @@ -526,17 +502,15 @@ describe('@lumino/widgets', () => { expect(SplitLayout.getStretch(widget)).to.equal(10); }); - it('should post a fit request to the parent', done => { + it('should post a fit request to the parent', () => { let parent = new Widget(); let widget = new Widget(); let layout = new LogSplitLayout({ renderer }); parent.layout = layout; layout.addWidget(widget); SplitLayout.setStretch(widget, 10); - requestAnimationFrame(() => { - expect(layout.methods).to.contain('onFitRequest'); - done(); - }); + MessageLoop.flush(); + expect(layout.methods).to.contain('onFitRequest'); }); }); }); From 66ee1b1aed47d98190d890f432479255af7072ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Collonval?= Date: Mon, 6 Mar 2023 15:19:55 +0100 Subject: [PATCH 5/6] Rename `overflowMenuVisibile` to `isVisible` Co-authored-by: Afshin Taylor Darian --- packages/widgets/src/menubar.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/widgets/src/menubar.ts b/packages/widgets/src/menubar.ts index 0d9a28ee2..23b3acd4c 100644 --- a/packages/widgets/src/menubar.ts +++ b/packages/widgets/src/menubar.ts @@ -1117,7 +1117,7 @@ export interface IOverflowMenuOptions { * * Defaults to `true`. */ - overflowMenuVisible: boolean; + isVisible: boolean; /** * Determines the title of the overflow menu. * From 0d4ecf0f2dad6e260dd26d605b6b580f21c66f73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Collonval?= Date: Mon, 6 Mar 2023 15:30:30 +0100 Subject: [PATCH 6/6] Propagate rename --- packages/widgets/src/menubar.ts | 12 ++++++------ packages/widgets/tests/src/menubar.spec.ts | 6 +++--- review/api/widgets.api.md | 2 +- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/widgets/src/menubar.ts b/packages/widgets/src/menubar.ts index 23b3acd4c..95f65a086 100644 --- a/packages/widgets/src/menubar.ts +++ b/packages/widgets/src/menubar.ts @@ -50,7 +50,7 @@ export class MenuBar extends Widget { forceY: true }; this._overflowMenuOptions = options.overflowMenuOptions || { - overflowMenuVisible: true + isVisible: true }; } @@ -433,7 +433,7 @@ export class MenuBar extends Widget { : 0; let length = this._overflowIndex > -1 ? this._overflowIndex : menus.length; let totalMenuSize = 0; - let overflowMenuVisible = false; + let isVisible = false; // Check that the overflow menu doesn't count length = this._overflowMenu !== null ? length - 1 : length; @@ -453,13 +453,13 @@ export class MenuBar extends Widget { totalMenuSize += this._menuItemSizes[i]; // Check if overflow menu is already rendered if (menus[i].title.label === this._overflowMenuOptions.title) { - overflowMenuVisible = true; + isVisible = true; length--; } } // Render overflow menu if needed and active - if (this._overflowMenuOptions.overflowMenuVisible) { - if (this._overflowIndex > -1 && !overflowMenuVisible) { + if (this._overflowMenuOptions.isVisible) { + if (this._overflowIndex > -1 && !isVisible) { // Create overflow menu if (this._overflowMenu === null) { const overflowMenuTitle = this._overflowMenuOptions.title ?? '...'; @@ -525,7 +525,7 @@ export class MenuBar extends Widget { * Calculate and update the current overflow index. */ private _updateOverflowIndex(): void { - if (!this._overflowMenuOptions.overflowMenuVisible) { + if (!this._overflowMenuOptions.isVisible) { return; } diff --git a/packages/widgets/tests/src/menubar.spec.ts b/packages/widgets/tests/src/menubar.spec.ts index aa159c2aa..50e5b7669 100644 --- a/packages/widgets/tests/src/menubar.spec.ts +++ b/packages/widgets/tests/src/menubar.spec.ts @@ -132,9 +132,9 @@ describe('@lumino/widgets', () => { bar.dispose(); }); - it('should accept only overflowMenuVisible option', () => { + it('should accept only isVisible option', () => { const bar = new MenuBar({ - overflowMenuOptions: { overflowMenuVisible: false } + overflowMenuOptions: { isVisible: false } }); expect(bar).to.be.an.instanceOf(MenuBar); @@ -937,7 +937,7 @@ describe('@lumino/widgets', () => { it('should not use the overflow menu', () => { let bar = createMenuBar({ - overflowMenuOptions: { overflowMenuVisible: false } + overflowMenuOptions: { isVisible: false } }); expect(bar.overflowIndex).to.equal(-1); expect(bar.overflowMenu).to.equal(null); diff --git a/review/api/widgets.api.md b/review/api/widgets.api.md index 16c8c9cf3..56b722ec5 100644 --- a/review/api/widgets.api.md +++ b/review/api/widgets.api.md @@ -584,7 +584,7 @@ export namespace GridLayout { // @public export interface IOverflowMenuOptions { - overflowMenuVisible: boolean; + isVisible: boolean; title?: string; }