From a2b7a21a95c0f1c40f2f2ce182bb93e28d6622d1 Mon Sep 17 00:00:00 2001 From: Brandy Carney Date: Fri, 19 Feb 2016 16:18:31 -0500 Subject: [PATCH 1/5] feat(menu): grab the menu by side only if it is enabled this removes the need to pass an id when you enable one menu references #5535 --- ionic/components/menu/menu-controller.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ionic/components/menu/menu-controller.ts b/ionic/components/menu/menu-controller.ts index 1c1fd69ac3..5865265cfb 100644 --- a/ionic/components/menu/menu-controller.ts +++ b/ionic/components/menu/menu-controller.ts @@ -263,7 +263,7 @@ export class MenuController { if (menu) return menu; // not found by "id", next try by "side" - menu = this._menus.find(m => m.side === menuId); + menu = this._menus.find(m => m.side === menuId && m._isEnabled); if (menu) return menu; } From f0b58388801daad1df687cb404d904e81c1b48a8 Mon Sep 17 00:00:00 2001 From: Brandy Carney Date: Fri, 19 Feb 2016 16:18:55 -0500 Subject: [PATCH 2/5] test(menu): update test to use 3 menus and menu toggle by side references #5535 --- .../menu/test/enable-disable/index.ts | 11 +++++-- .../menu/test/enable-disable/main.html | 29 ++++++++++++++++++- .../menu/test/enable-disable/page1.html | 6 ++-- .../menu/test/enable-disable/page2.html | 6 ++-- 4 files changed, 40 insertions(+), 12 deletions(-) diff --git a/ionic/components/menu/test/enable-disable/index.ts b/ionic/components/menu/test/enable-disable/index.ts index 947ec1c699..24255c5f2a 100644 --- a/ionic/components/menu/test/enable-disable/index.ts +++ b/ionic/components/menu/test/enable-disable/index.ts @@ -24,7 +24,7 @@ class E2EApp { constructor(app: IonicApp, menu: MenuController) { this.app = app; this.menu = menu; - + this.page1 = Page1; this.page2 = Page2; @@ -39,13 +39,18 @@ class E2EApp { } menu1Active() { - this.activeMenu = 'menu1'; this.menu.enable(true, 'menu1'); this.menu.enable(false, 'menu2'); + this.menu.enable(false, 'menu3'); } menu2Active() { - this.activeMenu = 'menu2'; this.menu.enable(false, 'menu1'); this.menu.enable(true, 'menu2'); + this.menu.enable(false, 'menu3'); + } + menu3Active() { + this.menu.enable(false, 'menu1'); + this.menu.enable(false, 'menu2'); + this.menu.enable(true, 'menu3'); } } diff --git a/ionic/components/menu/test/enable-disable/main.html b/ionic/components/menu/test/enable-disable/main.html index 031d8a8998..c3a4f5e835 100644 --- a/ionic/components/menu/test/enable-disable/main.html +++ b/ionic/components/menu/test/enable-disable/main.html @@ -9,6 +9,7 @@ + @@ -33,7 +34,8 @@ - + + @@ -48,5 +50,30 @@ + + + + Menu 3 + + + + + + + + + + + + + + + diff --git a/ionic/components/menu/test/enable-disable/page1.html b/ionic/components/menu/test/enable-disable/page1.html index 842e288465..2192d0834d 100644 --- a/ionic/components/menu/test/enable-disable/page1.html +++ b/ionic/components/menu/test/enable-disable/page1.html @@ -1,6 +1,6 @@ - @@ -12,10 +12,8 @@

Page 1

-

Active Menu: {{ activeMenu }}

-

- +

This page has two left menus, but only one is active at a time.

diff --git a/ionic/components/menu/test/enable-disable/page2.html b/ionic/components/menu/test/enable-disable/page2.html index fa3317b687..a5841a3b32 100644 --- a/ionic/components/menu/test/enable-disable/page2.html +++ b/ionic/components/menu/test/enable-disable/page2.html @@ -1,6 +1,6 @@ - @@ -12,10 +12,8 @@

Page 2

-

Active Menu: {{ activeMenu }}

-

- +

This page has two left menus, but only one is active at a time.

From 5f31df05deee3013b7c3ce6f95c5b66286b52224 Mon Sep 17 00:00:00 2001 From: Brandy Carney Date: Fri, 19 Feb 2016 20:36:05 -0500 Subject: [PATCH 3/5] test(menu): add karma tests --- ionic/components/menu/test/menu.spec.ts | 126 ++++++++++++++++++++++++ 1 file changed, 126 insertions(+) create mode 100644 ionic/components/menu/test/menu.spec.ts diff --git a/ionic/components/menu/test/menu.spec.ts b/ionic/components/menu/test/menu.spec.ts new file mode 100644 index 0000000000..ae085e8841 --- /dev/null +++ b/ionic/components/menu/test/menu.spec.ts @@ -0,0 +1,126 @@ +import {MenuController, Menu} from '../../../../ionic/ionic'; + +export function run() { + describe('MenuController', () => { + + describe('get', () => { + + /* + * Should not get menus + */ + it('should not get a menu if no menus', () => { + let menu = menuCtrl.get(); + expect(menu).toEqual(null); + }); + + it('should not get a menu with an id if no menus', () => { + let menu = menuCtrl.get('myid'); + expect(menu).toEqual(null); + }); + + it('should not get a menu with a side if no menus', () => { + let menu = menuCtrl.get('left'); + expect(menu).toEqual(null); + }); + + it('should not get a menu with a right side if no menus', () => { + let menu = menuCtrl.get('right'); + expect(menu).toEqual(null); + }); + + // this is grabbing an id when the menu doesn't exist + // this should not be working + it('should not get the menu by id without id', () => { + let menuById = mockMenu(); + menuCtrl.register(menuById); + + let menu = menuCtrl.get('myMenu'); + // setting this to null should be a success + expect(menu).toEqual(menuById); + }); + + /* + * Should get menus + */ + it('should get the menu by left', () => { + let leftMenu = mockMenu(); + menuCtrl.register(leftMenu); + + let menu = menuCtrl.get('left'); + expect(menu).toEqual(leftMenu); + }); + + it('should get the menu by left with side', () => { + let leftMenu = mockMenu(); + leftMenu.side = 'left'; + menuCtrl.register(leftMenu); + + let menu = menuCtrl.get('left'); + expect(menu).toEqual(leftMenu); + }); + + it('should get the menu by left with id', () => { + let menuById = mockMenu(); + menuById.id = 'myMenu'; + menuCtrl.register(menuById); + + let menu = menuCtrl.get('left'); + expect(menu).toEqual(menuById); + }); + + it('should get the menu by id with id', () => { + let menuById = mockMenu(); + menuById.id = 'myMenu'; + menuCtrl.register(menuById); + + let menu = menuCtrl.get('myMenu'); + expect(menu).toEqual(menuById); + }); + + + }); + + describe('toggle', () => { + + /* + * Should not toggle menus + */ + it('should not toggle the menu if disabled', () => { + let disabledMenu = mockMenu(); + disabledMenu.enabled = false; + menuCtrl.register(disabledMenu); + + let menu = menuCtrl.get(); + let toggle = menu.toggle(); + expect(toggle).toEqual(null); + }); + + }); + + it('should register a menu', () => { + let menu = mockMenu(); + menuCtrl.register(menu); + expect(menuCtrl.getMenus().length).toEqual(1); + + let menu2 = mockMenu(); + menuCtrl.register(menu2); + expect(menuCtrl.getMenus().length).toEqual(2); + + menuCtrl.unregister(menu2); + menuCtrl.unregister(menu); + + expect(menuCtrl.getMenus().length).toEqual(0); + }); + + let menuCtrl: MenuController; + + beforeEach(() => { + menuCtrl = new MenuController(); + }); + + function mockMenu(): Menu { + return new Menu(null, null, null, null, null, null, null); + } + + }); +} From 8564d79a7ded1cb79d13151fc62a7689e0db2748 Mon Sep 17 00:00:00 2001 From: Brandy Carney Date: Fri, 19 Feb 2016 20:36:27 -0500 Subject: [PATCH 4/5] fix(menu): fix enabled check references #5535 --- ionic/components/menu/menu-controller.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ionic/components/menu/menu-controller.ts b/ionic/components/menu/menu-controller.ts index 5865265cfb..353f2a1a96 100644 --- a/ionic/components/menu/menu-controller.ts +++ b/ionic/components/menu/menu-controller.ts @@ -263,7 +263,7 @@ export class MenuController { if (menu) return menu; // not found by "id", next try by "side" - menu = this._menus.find(m => m.side === menuId && m._isEnabled); + menu = this._menus.find(m => m.side === menuId && m.enabled); if (menu) return menu; } From 004e63555e7b5c07d864327471853277a8a28ecf Mon Sep 17 00:00:00 2001 From: Adam Bradley Date: Fri, 19 Feb 2016 23:37:31 -0600 Subject: [PATCH 5/5] refactor(menu): improve menu get lookup If a `menuId` is not provided then it'll return the first menu found. If a `menuId` is `left` or `right`, then it'll return the enabled menu on that side. Otherwise, if a `menuId` is provided, then it'll try to find the menu using the menu's `id` property. If a menu is not found then it'll return `null`. If a menu id was provided, but was not found, it will not fallback to finding any menu. Closes #5535 --- ionic/components/menu/menu-controller.ts | 36 ++- ionic/components/menu/menu.ts | 16 +- ionic/components/menu/test/menu.spec.ts | 278 ++++++++++++++++------- 3 files changed, 241 insertions(+), 89 deletions(-) diff --git a/ionic/components/menu/menu-controller.ts b/ionic/components/menu/menu-controller.ts index 353f2a1a96..a51359e2e3 100644 --- a/ionic/components/menu/menu-controller.ts +++ b/ionic/components/menu/menu-controller.ts @@ -207,8 +207,9 @@ export class MenuController { /** * Used to enable or disable a menu. For example, there could be multiple - * left menus, but only one of them should be able to be dragged open. - * @param {boolean} shouldEnable True if it should be enabled, false if not. + * left menus, but only one of them should be able to be opened at the same + * time. If there are multiple menus on the same side, then enabling one menu + * will also automatically disable all the others that are on the same side. * @param {string} [menuId] Optionally get the menu by its id, or side. * @return {Menu} Returns the instance of the menu, which is useful for chaining. */ @@ -249,24 +250,37 @@ export class MenuController { } /** - * Used to get a menu instance. If a `menuId` is not provided then it'll return - * the first menu found. If a `menuId` is provided, then it'll first try to find - * the menu using the menu's `id` attribute. If a menu is not found using the `id` - * attribute, then it'll try to find the menu by its `side` name. + * Used to get a menu instance. If a `menuId` is not provided then it'll + * return the first menu found. If a `menuId` is `left` or `right`, then + * it'll return the enabled menu on that side. Otherwise, if a `menuId` is + * provided, then it'll try to find the menu using the menu's `id` + * property. If a menu is not found then it'll return `null`. * @param {string} [menuId] Optionally get the menu by its id, or side. * @return {Menu} Returns the instance of the menu if found, otherwise `null`. */ get(menuId?: string): Menu { - if (menuId) { - // first try by "id" - let menu = this._menus.find(m => m.id === menuId); - if (menu) return menu; + var menu: Menu; - // not found by "id", next try by "side" + if (menuId === 'left' || menuId === 'right') { + // there could be more than one menu on the same side + // so first try to get the enabled one menu = this._menus.find(m => m.side === menuId && m.enabled); if (menu) return menu; + + // didn't find a menu side that is enabled + // so try to get the first menu side found + return this._menus.find(m => m.side === menuId) || null; + + } else if (menuId) { + // the menuId was not left or right + // so try to get the menu by its "id" + return this._menus.find(m => m.id === menuId) || null; } + // return the first enabled menu + menu = this._menus.find(m => m.enabled); + if (menu) return menu; + // get the first menu in the array, if one exists return (this._menus.length ? this._menus[0] : null); } diff --git a/ionic/components/menu/menu.ts b/ionic/components/menu/menu.ts index 87104a2cf3..af5bddb6bc 100644 --- a/ionic/components/menu/menu.ts +++ b/ionic/components/menu/menu.ts @@ -370,15 +370,29 @@ export class Menu extends Ion { /** * Used to enable or disable a menu. For example, there could be multiple - * left menus, but only one of them should be able to be dragged open. + * left menus, but only one of them should be able to be opened at the same + * time. If there are multiple menus on the same side, then enabling one menu + * will also automatically disable all the others that are on the same side. * @param {boolean} shouldEnable True if it should be enabled, false if not. * @return {Menu} Returns the instance of the menu, which is useful for chaining. */ enable(shouldEnable: boolean): Menu { this.enabled = shouldEnable; if (!shouldEnable && this.isOpen) { + // close if this menu is open, and should not be enabled this.close(); } + + if (shouldEnable) { + // if this menu should be enabled + // then find all the other menus on this same side + // and automatically disable other same side menus + let sameSideMenus = this._menuCtrl + .getMenus() + .filter(m => m.side === this.side && m !== this) + .map(m => m.enabled = false); + } + return this; } diff --git a/ionic/components/menu/test/menu.spec.ts b/ionic/components/menu/test/menu.spec.ts index ae085e8841..5d79fc94cc 100644 --- a/ionic/components/menu/test/menu.spec.ts +++ b/ionic/components/menu/test/menu.spec.ts @@ -3,112 +3,236 @@ import {MenuController, Menu} from '../../../../ionic/ionic'; export function run() { describe('MenuController', () => { - describe('get', () => { + describe('get() without menuId', () => { - /* - * Should not get menus - */ it('should not get a menu if no menus', () => { let menu = menuCtrl.get(); expect(menu).toEqual(null); }); - it('should not get a menu with an id if no menus', () => { + it('should get the only menu', () => { + let someMenu = mockMenu(); + menuCtrl.register(someMenu); + + let menu = menuCtrl.get(); + expect(menu).toEqual(someMenu); + }); + + it('should get the only menu if menuId === ""', () => { + let someMenu = mockMenu(); + menuCtrl.register(someMenu); + + let menu = menuCtrl.get(''); + expect(menu).toEqual(someMenu); + }); + + it('should get the enabled menu when multiple menus', () => { + let someMenu1 = mockMenu(); + someMenu1.enabled = false; + menuCtrl.register(someMenu1); + + let someMenu2 = mockMenu(); + someMenu2.enabled = true; + menuCtrl.register(someMenu2); + + let menu = menuCtrl.get(); + expect(menu).toEqual(someMenu2); + }); + + }); + + describe('get() by id', () => { + + it('should be null if no menus', () => { let menu = menuCtrl.get('myid'); expect(menu).toEqual(null); }); - - it('should not get a menu with a side if no menus', () => { + + it('should be null if no matching menus with id', () => { + let someMenu = mockMenu(); + someMenu.id = 'whatever'; + menuCtrl.register(someMenu); + + let menu = menuCtrl.get('myMenu'); + expect(menu).toEqual(null); + }); + + it('should get the menu by id with matching id', () => { + let someMenu = mockMenu(); + someMenu.id = 'myMenu'; + menuCtrl.register(someMenu); + + let menu = menuCtrl.get('myMenu'); + expect(menu).toEqual(someMenu); + }); + + it('should get the menu by id with left', () => { + let someMenu = mockMenu(); + someMenu.id = 'myMenu'; + someMenu.side = 'left'; + menuCtrl.register(someMenu); + + let menu = menuCtrl.get('myMenu'); + expect(menu).toEqual(someMenu); + }); + + it('should get the menu by id with matching id when multiple menus', () => { + let someMenu1 = mockMenu(); + someMenu1.id = 'myMenu1'; + menuCtrl.register(someMenu1); + + let someMenu2 = mockMenu(); + someMenu2.id = 'myMenu2'; + menuCtrl.register(someMenu2); + + let menu = menuCtrl.get('myMenu1'); + expect(menu).toEqual(someMenu1); + + menu = menuCtrl.get('myMenu2'); + expect(menu).toEqual(someMenu2); + }); + + }); + + describe('get() by side', () => { + + it('should not get a menu with a left side if no menus', () => { let menu = menuCtrl.get('left'); expect(menu).toEqual(null); - }); - + }); + it('should not get a menu with a right side if no menus', () => { let menu = menuCtrl.get('right'); expect(menu).toEqual(null); - }); - - // this is grabbing an id when the menu doesn't exist - // this should not be working - it('should not get the menu by id without id', () => { - let menuById = mockMenu(); - menuCtrl.register(menuById); - - let menu = menuCtrl.get('myMenu'); - // setting this to null should be a success - expect(menu).toEqual(menuById); - }); - - /* - * Should get menus - */ - it('should get the menu by left', () => { - let leftMenu = mockMenu(); - menuCtrl.register(leftMenu); - - let menu = menuCtrl.get('left'); - expect(menu).toEqual(leftMenu); - }); - - it('should get the menu by left with side', () => { - let leftMenu = mockMenu(); - leftMenu.side = 'left'; - menuCtrl.register(leftMenu); - - let menu = menuCtrl.get('left'); - expect(menu).toEqual(leftMenu); }); - - it('should get the menu by left with id', () => { - let menuById = mockMenu(); - menuById.id = 'myMenu'; - menuCtrl.register(menuById); - + + it('should get the only left menu', () => { + let someMenu = mockMenu(); + someMenu.side = 'left'; + menuCtrl.register(someMenu); + let menu = menuCtrl.get('left'); - expect(menu).toEqual(menuById); - }); + expect(menu).toEqual(someMenu); + }); - it('should get the menu by id with id', () => { - let menuById = mockMenu(); - menuById.id = 'myMenu'; - menuCtrl.register(menuById); - - let menu = menuCtrl.get('myMenu'); - expect(menu).toEqual(menuById); - }); + it('should get the enabled left menu', () => { + let someMenu1 = mockMenu(); + someMenu1.side = 'left'; + someMenu1.enabled = false; + menuCtrl.register(someMenu1); + let someMenu2 = mockMenu(); + someMenu2.side = 'left'; + someMenu2.enabled = true; + menuCtrl.register(someMenu2); + + let menu = menuCtrl.get('left'); + expect(menu).toEqual(someMenu2); + }); + + it('should get the first left menu when all are disabled', () => { + let someMenu1 = mockMenu(); + someMenu1.side = 'left'; + someMenu1.enabled = false; + menuCtrl.register(someMenu1); + + let someMenu2 = mockMenu(); + someMenu2.side = 'left'; + someMenu2.enabled = false; + menuCtrl.register(someMenu2); + + let menu = menuCtrl.get('left'); + expect(menu).toEqual(someMenu1); + }); + + it('should get the only right menu', () => { + let someMenu = mockMenu(); + someMenu.side = 'right'; + menuCtrl.register(someMenu); + + let menu = menuCtrl.get('right'); + expect(menu).toEqual(someMenu); + }); + + it('should get the menu by left with id', () => { + let someMenu = mockMenu(); + someMenu.id = 'myMenu'; + someMenu.side = 'left'; + menuCtrl.register(someMenu); + + let menu = menuCtrl.get('left'); + expect(menu).toEqual(someMenu); + }); + + }); + + describe('enable()', () => { + + it('should enable a menu', () => { + let someMenu = mockMenu(); + someMenu.enabled = true; + menuCtrl.register(someMenu); + someMenu._menuCtrl = menuCtrl; + + let menu = menuCtrl.enable(true); + expect(menu.enabled).toEqual(true); + + menu = menuCtrl.enable(false); + expect(menu.enabled).toEqual(false); + }); + + it('should be only one enabled menu on the same side', () => { + let someMenu1 = mockMenu(); + someMenu1.enabled = true; + someMenu1.side = 'left'; + someMenu1.id = 'menu1'; + someMenu1._menuCtrl = menuCtrl; + menuCtrl.register(someMenu1); + + let someMenu2 = mockMenu(); + someMenu2.enabled = false; + someMenu2.side = 'left'; + someMenu2.id = 'menu2'; + someMenu2._menuCtrl = menuCtrl; + menuCtrl.register(someMenu2); + + let someMenu3 = mockMenu(); + someMenu3.enabled = true; + someMenu3.side = 'right'; + someMenu3.id = 'menu2'; + someMenu3._menuCtrl = menuCtrl; + menuCtrl.register(someMenu3); + + menuCtrl.enable(true, 'menu1'); + expect(someMenu1.enabled).toEqual(true); + expect(someMenu2.enabled).toEqual(false); + expect(someMenu3.enabled).toEqual(true); + + menuCtrl.enable(true, 'menu2'); + expect(someMenu1.enabled).toEqual(false); + expect(someMenu2.enabled).toEqual(true); + expect(someMenu3.enabled).toEqual(true); + + menuCtrl.enable(true, 'menu1'); + expect(someMenu1.enabled).toEqual(true); + expect(someMenu2.enabled).toEqual(false); + expect(someMenu3.enabled).toEqual(true); + }); }); - - describe('toggle', () => { - - /* - * Should not toggle menus - */ - it('should not toggle the menu if disabled', () => { - let disabledMenu = mockMenu(); - disabledMenu.enabled = false; - menuCtrl.register(disabledMenu); - - let menu = menuCtrl.get(); - let toggle = menu.toggle(); - expect(toggle).toEqual(null); - }); - - }); it('should register a menu', () => { let menu = mockMenu(); menuCtrl.register(menu); expect(menuCtrl.getMenus().length).toEqual(1); - + let menu2 = mockMenu(); menuCtrl.register(menu2); expect(menuCtrl.getMenus().length).toEqual(2); - + menuCtrl.unregister(menu2); menuCtrl.unregister(menu); - + expect(menuCtrl.getMenus().length).toEqual(0); }); @@ -117,7 +241,7 @@ export function run() { beforeEach(() => { menuCtrl = new MenuController(); }); - + function mockMenu(): Menu { return new Menu(null, null, null, null, null, null, null); }