From 004e63555e7b5c07d864327471853277a8a28ecf Mon Sep 17 00:00:00 2001 From: Adam Bradley Date: Fri, 19 Feb 2016 23:37:31 -0600 Subject: [PATCH] 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 | 274 ++++++++++++++++------- 3 files changed, 239 insertions(+), 87 deletions(-) diff --git a/ionic/components/menu/menu-controller.ts b/ionic/components/menu/menu-controller.ts index 353f2a1a967..a51359e2e3c 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 87104a2cf34..af5bddb6bc6 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 ae085e8841a..5d79fc94cc4 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); - + }); + + it('should get the only left menu', () => { + let someMenu = mockMenu(); + someMenu.side = 'left'; + menuCtrl.register(someMenu); + + let menu = menuCtrl.get('left'); + expect(menu).toEqual(someMenu); + }); + + 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(leftMenu); - }); - - it('should get the menu by left with side', () => { - let leftMenu = mockMenu(); - leftMenu.side = 'left'; - menuCtrl.register(leftMenu); - + 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(leftMenu); + 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 menuById = mockMenu(); - menuById.id = 'myMenu'; - menuCtrl.register(menuById); - + let someMenu = mockMenu(); + someMenu.id = 'myMenu'; + someMenu.side = 'left'; + menuCtrl.register(someMenu); + 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); - }); + 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); }