From 5c7108f53bd861369c3b3b08f2de475fb2a93929 Mon Sep 17 00:00:00 2001 From: Diana Petcheva Date: Mon, 31 Mar 2025 08:47:32 +0300 Subject: [PATCH 01/11] refactor(ui5-menu): remove references to parent --- packages/main/src/Menu.ts | 79 +++++++++++------------- packages/main/src/MenuItem.ts | 85 +++++++++++++++++++++++++- packages/main/src/MenuItemTemplate.tsx | 5 +- packages/main/src/MenuTemplate.tsx | 1 + 4 files changed, 123 insertions(+), 47 deletions(-) diff --git a/packages/main/src/Menu.ts b/packages/main/src/Menu.ts index c960337d87de..a820d43296af 100644 --- a/packages/main/src/Menu.ts +++ b/packages/main/src/Menu.ts @@ -7,6 +7,7 @@ import { isLeft, isRight, isEnter, + isSpace, isTabNext, isTabPrevious, isDown, @@ -285,18 +286,6 @@ class Menu extends UI5Element { item.selected = true; } - _closeItemSubMenu(item: MenuItem) { - if (item && item._popover) { - const openedSibling = item._menuItems.find(menuItem => menuItem._popover && menuItem._popover.open); - if (openedSibling) { - this._closeItemSubMenu(openedSibling); - } - - item._popover.open = false; - item.selected = false; - } - } - _itemMouseOver(e: MouseEvent) { if (isDesktop()) { // respect mouseover only on desktop @@ -326,10 +315,9 @@ class Menu extends UI5Element { clearTimeout(this._timeout); this._timeout = setTimeout(() => { - const opener = item.parentElement as MenuItem | Menu; - const openedSibling = opener && opener._menuItems.find(menuItem => menuItem._popover && menuItem._popover.open); - if (openedSibling) { - this._closeItemSubMenu(openedSibling); + const menuItems = this._menuItems; + if (menuItems.indexOf(item) > -1) { + menuItems.forEach(menuItem => { menuItem !== item && menuItem._close(); }); } this._openItemSubMenu(item); @@ -355,42 +343,45 @@ class Menu extends UI5Element { _itemKeyDown(e: KeyboardEvent) { const isTabNextPrevious = isTabNext(e) || isTabPrevious(e); - const item = e.target as MenuItem; - const parentElement = item.parentElement as MenuItem; - const shouldItemNavigation = isUp(e) || isDown(e); + const item = e.target as MenuItem; // Type assignment here is misleading, as item can also be EndContent + const menuItemInMenu = this._menuItems.indexOf(item) > -1; + const isItemNavigation = isUp(e) || isDown(e); + const isItemSelection = isEnter(e) || isSpace(e); + const isEndContentNavigation = isRight(e) || isLeft(e); const shouldOpenMenu = this.isRtl ? isLeft(e) : isRight(e); - const shouldCloseMenu = !shouldItemNavigation && !shouldOpenMenu && this._isInstanceOfMenuItem(parentElement); + const shouldCloseMenu = menuItemInMenu && !(isItemNavigation || isItemSelection || isEndContentNavigation); - if (this._isInstanceOfMenuItem(item)) { - if (isEnter(e) || isTabNextPrevious) { - e.preventDefault(); - } + if (!this._isInstanceOfMenuItem(item)) { + return; + } - if (isRight(e) || isLeft(e)) { - item._navigateToEndContent(isLeft(e)); - } + if (isEnter(e) || isTabNextPrevious) { + e.preventDefault(); + } - if (shouldOpenMenu) { - this._openItemSubMenu(item); - } else if ((shouldCloseMenu || isTabNextPrevious) && parentElement._popover) { - parentElement._popover.open = false; - parentElement.selected = false; - parentElement._popover.focusOpener(); - } - } else if (isUp(e)) { - this._navigateOutOfEndContent(parentElement); - } else if (isDown(e)) { - this._navigateOutOfEndContent(parentElement, true); + if (isEndContentNavigation) { + item._navigateToEndContent(isLeft(e)); + } + + if (shouldOpenMenu) { + this._openItemSubMenu(item); + } else if ((shouldCloseMenu || isTabNextPrevious)) { + this._close(); } } - _navigateOutOfEndContent(menuItem: MenuItem, isDownwards?: boolean) { - const opener = menuItem?.parentElement as MenuItem | Menu; - const currentIndex = opener._menuItems.indexOf(menuItem); - const nextItem = isDownwards ? opener._menuItems[currentIndex + 1] : opener._menuItems[currentIndex - 1]; - const itemToFocus = nextItem || opener._menuItems[currentIndex]; + _navigateOutOfEndContent(e: CustomEvent) { + const item = e.target as MenuItem; + const isLast = e.detail.isLast; + const itemIndex = this._menuItems.indexOf(item); + + if (itemIndex > -1) { + const nextItem = isLast ? this._menuItems[itemIndex + 1] : this._menuItems[itemIndex - 1]; + const itemToFocus = nextItem || this._menuItems[itemIndex]; + itemToFocus?.focus(); - itemToFocus.focus(); + e.stopPropagation(); + } } _beforePopoverOpen(e: CustomEvent) { diff --git a/packages/main/src/MenuItem.ts b/packages/main/src/MenuItem.ts index 8c3726b86249..c14460fac881 100644 --- a/packages/main/src/MenuItem.ts +++ b/packages/main/src/MenuItem.ts @@ -5,7 +5,17 @@ import type { AccessibilityAttributes, AriaHasPopup, AriaRole } from "@ui5/webco import event from "@ui5/webcomponents-base/dist/decorators/event-strict.js"; import slot from "@ui5/webcomponents-base/dist/decorators/slot.js"; import i18n from "@ui5/webcomponents-base/dist/decorators/i18n.js"; -import { isPhone } from "@ui5/webcomponents-base/dist/Device.js"; +import { + isLeft, + isRight, + isEnter, + isSpace, + isTabNext, + isTabPrevious, + isDown, + isUp, +} from "@ui5/webcomponents-base/dist/Keys.js"; +import { isDesktop, isPhone } from "@ui5/webcomponents-base/dist/Device.js"; import { renderFinished } from "@ui5/webcomponents-base/dist/Render.js"; import "@ui5/webcomponents-icons/dist/nav-back.js"; import type I18nBundle from "@ui5/webcomponents-base/dist/i18nBundle.js"; @@ -30,6 +40,8 @@ import menuItemCss from "./generated/themes/MenuItem.css.js"; type MenuBeforeOpenEventDetail = { item?: MenuItem }; type MenuBeforeCloseEventDetail = { escPressed: boolean }; +type MenuNavigateOutOfEndContentEventDetail = { isLast: boolean }; + type MenuItemAccessibilityAttributes = Pick & ListItemAccessibilityAttributes; /** @@ -88,6 +100,14 @@ type MenuItemAccessibilityAttributes = Pick !item.isSeparator); } + _itemMouseOver(e: MouseEvent) { + if (isDesktop()) { + // respect mouseover only on desktop + const item = e.target as MenuItem; + + if (this._isInstanceOfMenuItem(item)) { + item.focus(); + + const menuItems = this._menuItems; + if (menuItems.indexOf(item) > -1) { + menuItems.forEach(menuItem => { menuItem !== item && menuItem._close(); }); + } + } + } + } + + _itemKeyDown(e: KeyboardEvent) { + const item = e.target as MenuItem; + const itemInMenuItems = this._menuItems.indexOf(item) > -1; + const isTabNextPrevious = isTabNext(e) || isTabPrevious(e); + const isItemNavigation = isUp(e) || isDown(e); + const isItemSelection = isSpace(e) || isEnter(e); + const shouldOpenMenu = this.isRtl ? isLeft(e) : isRight(e); + const shouldCloseMenu = !(isItemNavigation || isItemSelection || shouldOpenMenu) || isTabNextPrevious; + + if (itemInMenuItems && shouldCloseMenu) { + this._close(); + this.focus(); + e.stopPropagation(); + } + } + + _endContentKeyDown(e: KeyboardEvent) { + const shouldNavigateOutOfEndContent = isUp(e) || isDown(e); + + if (shouldNavigateOutOfEndContent) { + this.fireDecoratorEvent("navigate-out", { isLast: isDown(e) }); + } + } + + _navigateOutOfEndContent(e: CustomEvent) { + const item = e.target as MenuItem; + const isLast = e.detail.isLast; + const menuItems = this._menuItems; + const itemIndex = menuItems.indexOf(item); + + if (itemIndex > -1) { + const nextItem = isLast ? menuItems[itemIndex + 1] : menuItems[itemIndex - 1]; + const itemToFocus = nextItem || menuItems[itemIndex]; + itemToFocus?.focus(); + + e.stopPropagation(); + } + } + _closeAll() { if (this._popover) { this._popover.open = false; @@ -393,6 +469,7 @@ class MenuItem extends ListItem implements IMenuItem { _close() { if (this._popover) { this._popover.open = false; + this._menuItems.forEach(item => item._close()); } this.selected = false; } @@ -431,6 +508,10 @@ class MenuItem extends ListItem implements IMenuItem { this.fireDecoratorEvent("close"); } + _isInstanceOfMenuItem(object: any): object is MenuItem { + return "isMenuItem" in object; + } + get isMenuItem(): boolean { return true; } diff --git a/packages/main/src/MenuItemTemplate.tsx b/packages/main/src/MenuItemTemplate.tsx index 15b5fd91b9ab..0d914021f764 100644 --- a/packages/main/src/MenuItemTemplate.tsx +++ b/packages/main/src/MenuItemTemplate.tsx @@ -46,7 +46,7 @@ function rightContent(this: MenuItem) { ); case this.hasEndContent: - return ; + return ; case !!this.additionalText: return ( diff --git a/packages/main/src/MenuTemplate.tsx b/packages/main/src/MenuTemplate.tsx index c4d4a7a10266..c50c4691ca19 100644 --- a/packages/main/src/MenuTemplate.tsx +++ b/packages/main/src/MenuTemplate.tsx @@ -54,6 +54,7 @@ export default function MenuTemplate(this: Menu) { onKeyDown={this._itemKeyDown} // handles event from slotted children onui5-close-menu={this._close} + onui5-navigate-out={this._navigateOutOfEndContent} > ) From 89d44641dae249352f60c97b61fb537fd19d7377 Mon Sep 17 00:00:00 2001 From: Diana Petcheva Date: Wed, 2 Apr 2025 13:30:54 +0300 Subject: [PATCH 02/11] fix: address comment --- packages/main/src/Menu.ts | 11 +++-------- packages/main/src/MenuItem.ts | 10 +++++----- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/packages/main/src/Menu.ts b/packages/main/src/Menu.ts index a820d43296af..c1790a45ce0c 100644 --- a/packages/main/src/Menu.ts +++ b/packages/main/src/Menu.ts @@ -25,8 +25,7 @@ import type { Timeout } from "@ui5/webcomponents-base/dist/types.js"; import { renderFinished } from "@ui5/webcomponents-base/dist/Render.js"; import DOMReferenceConverter from "@ui5/webcomponents-base/dist/converters/DOMReference.js"; import type ResponsivePopover from "./ResponsivePopover.js"; -import type MenuItem from "./MenuItem.js"; -import "./MenuItem.js"; +import MenuItem from "./MenuItem.js"; import "./MenuSeparator.js"; import type { ListItemClickEventDetail, @@ -291,7 +290,7 @@ class Menu extends UI5Element { // respect mouseover only on desktop const item = e.target as MenuItem; - if (this._isInstanceOfMenuItem(item)) { + if (MenuItem._isInstanceOfMenuItem(item)) { item.focus(); // Opens submenu with 300ms delay @@ -351,7 +350,7 @@ class Menu extends UI5Element { const shouldOpenMenu = this.isRtl ? isLeft(e) : isRight(e); const shouldCloseMenu = menuItemInMenu && !(isItemNavigation || isItemSelection || isEndContentNavigation); - if (!this._isInstanceOfMenuItem(item)) { + if (!MenuItem._isInstanceOfMenuItem(item)) { return; } @@ -411,10 +410,6 @@ class Menu extends UI5Element { this.open = false; this.fireDecoratorEvent("close"); } - - _isInstanceOfMenuItem(object: any): object is MenuItem { - return "isMenuItem" in object; - } } Menu.define(); diff --git a/packages/main/src/MenuItem.ts b/packages/main/src/MenuItem.ts index c14460fac881..33d1cb9e72ed 100644 --- a/packages/main/src/MenuItem.ts +++ b/packages/main/src/MenuItem.ts @@ -408,7 +408,7 @@ class MenuItem extends ListItem implements IMenuItem { // respect mouseover only on desktop const item = e.target as MenuItem; - if (this._isInstanceOfMenuItem(item)) { + if (MenuItem._isInstanceOfMenuItem(item)) { item.focus(); const menuItems = this._menuItems; @@ -508,13 +508,13 @@ class MenuItem extends ListItem implements IMenuItem { this.fireDecoratorEvent("close"); } - _isInstanceOfMenuItem(object: any): object is MenuItem { - return "isMenuItem" in object; - } - get isMenuItem(): boolean { return true; } + + static _isInstanceOfMenuItem(object: any): object is MenuItem { + return "isMenuItem" in object; + } } MenuItem.define(); From 448b0696766dcd7f3765dbff5b13cfcf8bbaa57d Mon Sep 17 00:00:00 2001 From: Diana Petcheva Date: Wed, 9 Apr 2025 18:21:39 +0300 Subject: [PATCH 03/11] chore: addressed review comments and small optimizations --- packages/main/cypress/specs/Menu.cy.tsx | 4 +- packages/main/src/Menu.ts | 57 +++++++++++++++---------- packages/main/src/MenuItem.ts | 54 ++++++++++++++--------- packages/main/src/MenuItemTemplate.tsx | 2 +- packages/main/src/MenuTemplate.tsx | 2 +- packages/main/test/pages/Menu.html | 6 +-- 6 files changed, 72 insertions(+), 53 deletions(-) diff --git a/packages/main/cypress/specs/Menu.cy.tsx b/packages/main/cypress/specs/Menu.cy.tsx index 9229c602b577..8ceb1427e0a9 100644 --- a/packages/main/cypress/specs/Menu.cy.tsx +++ b/packages/main/cypress/specs/Menu.cy.tsx @@ -569,9 +569,7 @@ describe("Menu interaction", () => { .should("have.attr", "accessible-name", "Select an option from the menu"); }); - /* The test is valid, but currently it is not stable. It will be reviewed further and stabilized afterwards. */ - - it.skip("Menu items - navigation in endContent", () => { + it("Menu items - navigation in endContent", () => { cy.mount( <> diff --git a/packages/main/src/Menu.ts b/packages/main/src/Menu.ts index c1790a45ce0c..d21b25e5ac5f 100644 --- a/packages/main/src/Menu.ts +++ b/packages/main/src/Menu.ts @@ -286,17 +286,19 @@ class Menu extends UI5Element { } _itemMouseOver(e: MouseEvent) { - if (isDesktop()) { - // respect mouseover only on desktop - const item = e.target as MenuItem; - - if (MenuItem._isInstanceOfMenuItem(item)) { - item.focus(); + if (!isDesktop()) { + return; + } - // Opens submenu with 300ms delay - this._startOpenTimeout(item); - } + const item = e.target as MenuItem; + if (!MenuItem._isInstanceOfMenuItem(item)) { + return; } + + item.focus(); + + // Opens submenu with 300ms delay + this._startOpenTimeout(item); } async focus(focusOptions?: FocusOptions): Promise { @@ -310,14 +312,23 @@ class Menu extends UI5Element { return super.focus(focusOptions); } + _closeOtherSubMenus(item: MenuItem) { + const menuItems = this._menuItems; + if (menuItems.indexOf(item) < 0) { + return; + } + menuItems.forEach(menuItem => { + if (menuItem !== item) { + menuItem._close(); + } + }); + } + _startOpenTimeout(item: MenuItem) { clearTimeout(this._timeout); this._timeout = setTimeout(() => { - const menuItems = this._menuItems; - if (menuItems.indexOf(item) > -1) { - menuItems.forEach(menuItem => { menuItem !== item && menuItem._close(); }); - } + this._closeOtherSubMenus(item); this._openItemSubMenu(item); }, MENU_OPEN_DELAY); @@ -342,7 +353,12 @@ class Menu extends UI5Element { _itemKeyDown(e: KeyboardEvent) { const isTabNextPrevious = isTabNext(e) || isTabPrevious(e); - const item = e.target as MenuItem; // Type assignment here is misleading, as item can also be EndContent + const item = e.target as MenuItem; + + if (!MenuItem._isInstanceOfMenuItem(item)) { + return; + } + const menuItemInMenu = this._menuItems.indexOf(item) > -1; const isItemNavigation = isUp(e) || isDown(e); const isItemSelection = isEnter(e) || isSpace(e); @@ -350,10 +366,6 @@ class Menu extends UI5Element { const shouldOpenMenu = this.isRtl ? isLeft(e) : isRight(e); const shouldCloseMenu = menuItemInMenu && !(isItemNavigation || isItemSelection || isEndContentNavigation); - if (!MenuItem._isInstanceOfMenuItem(item)) { - return; - } - if (isEnter(e) || isTabNextPrevious) { e.preventDefault(); } @@ -371,12 +383,13 @@ class Menu extends UI5Element { _navigateOutOfEndContent(e: CustomEvent) { const item = e.target as MenuItem; - const isLast = e.detail.isLast; - const itemIndex = this._menuItems.indexOf(item); + const shouldNavigateToNextItem = e.detail.shouldNavigateToNextItem; + const menuItems = this._menuItems; + const itemIndex = menuItems.indexOf(item); if (itemIndex > -1) { - const nextItem = isLast ? this._menuItems[itemIndex + 1] : this._menuItems[itemIndex - 1]; - const itemToFocus = nextItem || this._menuItems[itemIndex]; + const nextItem = shouldNavigateToNextItem ? menuItems[itemIndex + 1] : menuItems[itemIndex - 1]; + const itemToFocus = nextItem || menuItems[itemIndex]; itemToFocus?.focus(); e.stopPropagation(); diff --git a/packages/main/src/MenuItem.ts b/packages/main/src/MenuItem.ts index 33d1cb9e72ed..28dfcbfa3417 100644 --- a/packages/main/src/MenuItem.ts +++ b/packages/main/src/MenuItem.ts @@ -40,7 +40,7 @@ import menuItemCss from "./generated/themes/MenuItem.css.js"; type MenuBeforeOpenEventDetail = { item?: MenuItem }; type MenuBeforeCloseEventDetail = { escPressed: boolean }; -type MenuNavigateOutOfEndContentEventDetail = { isLast: boolean }; +type MenuNavigateOutOfEndContentEventDetail = { shouldNavigateToNextItem: boolean }; type MenuItemAccessibilityAttributes = Pick & ListItemAccessibilityAttributes; @@ -104,7 +104,7 @@ type MenuItemAccessibilityAttributes = Pick !item.isSeparator); } - _itemMouseOver(e: MouseEvent) { - if (isDesktop()) { - // respect mouseover only on desktop - const item = e.target as MenuItem; - - if (MenuItem._isInstanceOfMenuItem(item)) { - item.focus(); + _closeOtherSubMenus(item: MenuItem) { + const menuItems = this._menuItems; + if (menuItems.indexOf(item) === -1) { + return; + } - const menuItems = this._menuItems; - if (menuItems.indexOf(item) > -1) { - menuItems.forEach(menuItem => { menuItem !== item && menuItem._close(); }); - } + menuItems.forEach(menuItem => { + if (menuItem !== item) { + menuItem._close(); } + }); + } + + _itemMouseOver(e: MouseEvent) { + if (!isDesktop()) { + return; } + const item = e.target as MenuItem; + + if (!MenuItem._isInstanceOfMenuItem(item)) { + return; + } + item.focus(); + + this._closeOtherSubMenus(item); } _itemKeyDown(e: KeyboardEvent) { @@ -439,18 +451,18 @@ class MenuItem extends ListItem implements IMenuItem { const shouldNavigateOutOfEndContent = isUp(e) || isDown(e); if (shouldNavigateOutOfEndContent) { - this.fireDecoratorEvent("navigate-out", { isLast: isDown(e) }); + this.fireDecoratorEvent("exit-end-content", { shouldNavigateToNextItem: isDown(e) }); } } _navigateOutOfEndContent(e: CustomEvent) { const item = e.target as MenuItem; - const isLast = e.detail.isLast; + const shouldNavigateToNextItem = e.detail.shouldNavigateToNextItem; const menuItems = this._menuItems; const itemIndex = menuItems.indexOf(item); if (itemIndex > -1) { - const nextItem = isLast ? menuItems[itemIndex + 1] : menuItems[itemIndex - 1]; + const nextItem = shouldNavigateToNextItem ? menuItems[itemIndex + 1] : menuItems[itemIndex - 1]; const itemToFocus = nextItem || menuItems[itemIndex]; itemToFocus?.focus(); diff --git a/packages/main/src/MenuItemTemplate.tsx b/packages/main/src/MenuItemTemplate.tsx index 0d914021f764..de275dfd7bbb 100644 --- a/packages/main/src/MenuItemTemplate.tsx +++ b/packages/main/src/MenuItemTemplate.tsx @@ -127,7 +127,7 @@ function listItemPostContent(this: MenuItem) { onKeyDown={this._itemKeyDown} // handles event from slotted children onui5-close-menu={this._close} - onui5-navigate-out={this._navigateOutOfEndContent} + onui5-exit-end-content={this._navigateOutOfEndContent} > diff --git a/packages/main/src/MenuTemplate.tsx b/packages/main/src/MenuTemplate.tsx index c50c4691ca19..38bdfba5e6e9 100644 --- a/packages/main/src/MenuTemplate.tsx +++ b/packages/main/src/MenuTemplate.tsx @@ -54,7 +54,7 @@ export default function MenuTemplate(this: Menu) { onKeyDown={this._itemKeyDown} // handles event from slotted children onui5-close-menu={this._close} - onui5-navigate-out={this._navigateOutOfEndContent} + onui5-exit-end-content={this._navigateOutOfEndContent} > ) diff --git a/packages/main/test/pages/Menu.html b/packages/main/test/pages/Menu.html index 5ad370ad91e1..31f4187f0fd8 100644 --- a/packages/main/test/pages/Menu.html +++ b/packages/main/test/pages/Menu.html @@ -86,7 +86,7 @@ - + @@ -273,10 +273,6 @@ role: "menuitemcheckbox", }; - menuItemFromD.accessibilityAttributes = { - role: "menuitemcheckbox", - }; - \ No newline at end of file From cd768cf1ee8ae10765f18afe7d3eb2e9726a7910 Mon Sep 17 00:00:00 2001 From: Diana Petcheva Date: Wed, 9 Apr 2025 19:00:22 +0300 Subject: [PATCH 04/11] test: skip again, because even though it never fails locally, it still fails --- packages/main/cypress/specs/Menu.cy.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/main/cypress/specs/Menu.cy.tsx b/packages/main/cypress/specs/Menu.cy.tsx index 8ceb1427e0a9..9229c602b577 100644 --- a/packages/main/cypress/specs/Menu.cy.tsx +++ b/packages/main/cypress/specs/Menu.cy.tsx @@ -569,7 +569,9 @@ describe("Menu interaction", () => { .should("have.attr", "accessible-name", "Select an option from the menu"); }); - it("Menu items - navigation in endContent", () => { + /* The test is valid, but currently it is not stable. It will be reviewed further and stabilized afterwards. */ + + it.skip("Menu items - navigation in endContent", () => { cy.mount( <> From 3f1a8843d98b3766c32ea77adf21b7279d5748ca Mon Sep 17 00:00:00 2001 From: Diana Petcheva Date: Wed, 9 Apr 2025 19:47:06 +0300 Subject: [PATCH 05/11] chore: minor code style changes --- packages/main/src/Menu.ts | 1 + packages/main/src/MenuItem.ts | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/main/src/Menu.ts b/packages/main/src/Menu.ts index d21b25e5ac5f..001c22ea565b 100644 --- a/packages/main/src/Menu.ts +++ b/packages/main/src/Menu.ts @@ -317,6 +317,7 @@ class Menu extends UI5Element { if (menuItems.indexOf(item) < 0) { return; } + menuItems.forEach(menuItem => { if (menuItem !== item) { menuItem._close(); diff --git a/packages/main/src/MenuItem.ts b/packages/main/src/MenuItem.ts index 28dfcbfa3417..5ee9336dddb5 100644 --- a/packages/main/src/MenuItem.ts +++ b/packages/main/src/MenuItem.ts @@ -406,7 +406,7 @@ class MenuItem extends ListItem implements IMenuItem { _closeOtherSubMenus(item: MenuItem) { const menuItems = this._menuItems; - if (menuItems.indexOf(item) === -1) { + if (menuItems.indexOf(item) < 0) { return; } From 5baad708ffb5167c9e2940189077bd19902e4a57 Mon Sep 17 00:00:00 2001 From: Diana Petcheva Date: Wed, 16 Apr 2025 10:29:26 +0300 Subject: [PATCH 06/11] chore: comments --- packages/main/src/Menu.ts | 4 ++-- packages/main/src/MenuItem.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/main/src/Menu.ts b/packages/main/src/Menu.ts index 001c22ea565b..d81e7f10adf8 100644 --- a/packages/main/src/Menu.ts +++ b/packages/main/src/Menu.ts @@ -314,7 +314,7 @@ class Menu extends UI5Element { _closeOtherSubMenus(item: MenuItem) { const menuItems = this._menuItems; - if (menuItems.indexOf(item) < 0) { + if (!menuItems.includes(item)) { return; } @@ -360,7 +360,7 @@ class Menu extends UI5Element { return; } - const menuItemInMenu = this._menuItems.indexOf(item) > -1; + const menuItemInMenu = this._menuItems.includes(item); const isItemNavigation = isUp(e) || isDown(e); const isItemSelection = isEnter(e) || isSpace(e); const isEndContentNavigation = isRight(e) || isLeft(e); diff --git a/packages/main/src/MenuItem.ts b/packages/main/src/MenuItem.ts index 5ee9336dddb5..f8f3ca2c8368 100644 --- a/packages/main/src/MenuItem.ts +++ b/packages/main/src/MenuItem.ts @@ -406,7 +406,7 @@ class MenuItem extends ListItem implements IMenuItem { _closeOtherSubMenus(item: MenuItem) { const menuItems = this._menuItems; - if (menuItems.indexOf(item) < 0) { + if (!menuItems.includes(item)) { return; } @@ -433,7 +433,7 @@ class MenuItem extends ListItem implements IMenuItem { _itemKeyDown(e: KeyboardEvent) { const item = e.target as MenuItem; - const itemInMenuItems = this._menuItems.indexOf(item) > -1; + const itemInMenuItems = this._menuItems.includes(item); const isTabNextPrevious = isTabNext(e) || isTabPrevious(e); const isItemNavigation = isUp(e) || isDown(e); const isItemSelection = isSpace(e) || isEnter(e); From ccfada152325cec907ad3c954194dcedf7538f61 Mon Sep 17 00:00:00 2001 From: Diana Petcheva Date: Tue, 29 Apr 2025 15:18:53 +0300 Subject: [PATCH 07/11] chore: address comments --- packages/main/src/Menu.ts | 6 +++--- packages/main/src/MenuItem.ts | 14 +++++++++----- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/packages/main/src/Menu.ts b/packages/main/src/Menu.ts index 88fabfcbc03c..e49fa344a8b3 100644 --- a/packages/main/src/Menu.ts +++ b/packages/main/src/Menu.ts @@ -25,7 +25,7 @@ import type { Timeout } from "@ui5/webcomponents-base/dist/types.js"; import { renderFinished } from "@ui5/webcomponents-base/dist/Render.js"; import DOMReferenceConverter from "@ui5/webcomponents-base/dist/converters/DOMReference.js"; import type ResponsivePopover from "./ResponsivePopover.js"; -import MenuItem from "./MenuItem.js"; +import MenuItem, { isInstanceOfMenuItem } from "./MenuItem.js"; import type PopoverHorizontalAlign from "./types/PopoverHorizontalAlign.js"; import "./MenuSeparator.js"; import type { @@ -300,7 +300,7 @@ class Menu extends UI5Element { } const item = e.target as MenuItem; - if (!MenuItem._isInstanceOfMenuItem(item)) { + if (!isInstanceOfMenuItem(item)) { return; } @@ -365,7 +365,7 @@ class Menu extends UI5Element { const isTabNextPrevious = isTabNext(e) || isTabPrevious(e); const item = e.target as MenuItem; - if (!MenuItem._isInstanceOfMenuItem(item)) { + if (!isInstanceOfMenuItem(item)) { return; } diff --git a/packages/main/src/MenuItem.ts b/packages/main/src/MenuItem.ts index f8f3ca2c8368..620bec0dff3e 100644 --- a/packages/main/src/MenuItem.ts +++ b/packages/main/src/MenuItem.ts @@ -423,7 +423,7 @@ class MenuItem extends ListItem implements IMenuItem { } const item = e.target as MenuItem; - if (!MenuItem._isInstanceOfMenuItem(item)) { + if (!isInstanceOfMenuItem(item)) { return; } item.focus(); @@ -523,14 +523,14 @@ class MenuItem extends ListItem implements IMenuItem { get isMenuItem(): boolean { return true; } - - static _isInstanceOfMenuItem(object: any): object is MenuItem { - return "isMenuItem" in object; - } } MenuItem.define(); +const isInstanceOfMenuItem = (object: any): object is MenuItem => { + return "isMenuItem" in object && object.isMenuItem; +}; + export default MenuItem; export type { @@ -538,3 +538,7 @@ export type { MenuBeforeOpenEventDetail, MenuItemAccessibilityAttributes, }; + +export { + isInstanceOfMenuItem, +}; From b2984236a964c7c86ccef77a540939dc84478318 Mon Sep 17 00:00:00 2001 From: Diana Petcheva Date: Fri, 9 May 2025 11:23:46 +0300 Subject: [PATCH 08/11] chore: fix imports --- packages/main/src/Menu.ts | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/packages/main/src/Menu.ts b/packages/main/src/Menu.ts index e49fa344a8b3..3c65fc0b7fee 100644 --- a/packages/main/src/Menu.ts +++ b/packages/main/src/Menu.ts @@ -25,7 +25,8 @@ import type { Timeout } from "@ui5/webcomponents-base/dist/types.js"; import { renderFinished } from "@ui5/webcomponents-base/dist/Render.js"; import DOMReferenceConverter from "@ui5/webcomponents-base/dist/converters/DOMReference.js"; import type ResponsivePopover from "./ResponsivePopover.js"; -import MenuItem, { isInstanceOfMenuItem } from "./MenuItem.js"; +import type MenuItemT from "./MenuItem.js"; +import * as MenuItem from "./MenuItem.js"; import type PopoverHorizontalAlign from "./types/PopoverHorizontalAlign.js"; import "./MenuSeparator.js"; import type { @@ -53,11 +54,11 @@ interface IMenuItem extends UI5Element { } type MenuItemClickEventDetail = { - item: MenuItem, + item: MenuItemT, text: string, } -type MenuBeforeOpenEventDetail = { item?: MenuItem }; +type MenuBeforeOpenEventDetail = { item?: MenuItemT }; type MenuBeforeCloseEventDetail = { escPressed: boolean }; /** @@ -260,7 +261,7 @@ class Menu extends UI5Element { } get _menuItems() { - return this.items.filter((item): item is MenuItem => !item.isSeparator); + return this.items.filter((item): item is MenuItemT => !item.isSeparator); } get acessibleNameText() { @@ -279,7 +280,7 @@ class Menu extends UI5Element { this.open = false; } - _openItemSubMenu(item: MenuItem) { + _openItemSubMenu(item: MenuItemT) { clearTimeout(this._timeout); if (!item._popover || item._popover.open) { @@ -299,8 +300,8 @@ class Menu extends UI5Element { return; } - const item = e.target as MenuItem; - if (!isInstanceOfMenuItem(item)) { + const item = e.target as MenuItemT; + if (!MenuItem.isInstanceOfMenuItem(item)) { return; } @@ -321,7 +322,7 @@ class Menu extends UI5Element { return super.focus(focusOptions); } - _closeOtherSubMenus(item: MenuItem) { + _closeOtherSubMenus(item: MenuItemT) { const menuItems = this._menuItems; if (!menuItems.includes(item)) { return; @@ -334,7 +335,7 @@ class Menu extends UI5Element { }); } - _startOpenTimeout(item: MenuItem) { + _startOpenTimeout(item: MenuItemT) { clearTimeout(this._timeout); this._timeout = setTimeout(() => { @@ -345,7 +346,7 @@ class Menu extends UI5Element { } _itemClick(e: CustomEvent) { - const item = e.detail.item as MenuItem; + const item = e.detail.item as MenuItemT; if (!item._popover) { const prevented = !this.fireDecoratorEvent("item-click", { @@ -363,9 +364,9 @@ class Menu extends UI5Element { _itemKeyDown(e: KeyboardEvent) { const isTabNextPrevious = isTabNext(e) || isTabPrevious(e); - const item = e.target as MenuItem; + const item = e.target as MenuItemT; - if (!isInstanceOfMenuItem(item)) { + if (!MenuItem.isInstanceOfMenuItem(item)) { return; } @@ -392,7 +393,7 @@ class Menu extends UI5Element { } _navigateOutOfEndContent(e: CustomEvent) { - const item = e.target as MenuItem; + const item = e.target as MenuItemT; const shouldNavigateToNextItem = e.detail.shouldNavigateToNextItem; const menuItems = this._menuItems; const itemIndex = menuItems.indexOf(item); From 0a3b4666dc94c541e52ef744528d0e9678e4fbdb Mon Sep 17 00:00:00 2001 From: Diana Petcheva Date: Fri, 9 May 2025 11:28:47 +0300 Subject: [PATCH 09/11] fix: linter error --- packages/main/src/MenuItem.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/main/src/MenuItem.ts b/packages/main/src/MenuItem.ts index 278a9a9c4440..c7e28bfb9b23 100644 --- a/packages/main/src/MenuItem.ts +++ b/packages/main/src/MenuItem.ts @@ -524,7 +524,7 @@ class MenuItem extends ListItem implements IMenuItem { MenuItem.define(); const isInstanceOfMenuItem = (object: any): object is MenuItem => { - return "isMenuItem" in object && object.isMenuItem; + return "isMenuItem" in object; }; export default MenuItem; From 9778b53e33d14815d5d34251ba56909e0d532535 Mon Sep 17 00:00:00 2001 From: Diana Petcheva Date: Mon, 12 May 2025 13:26:02 +0300 Subject: [PATCH 10/11] chore: revert import changes --- packages/main/src/Menu.ts | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/packages/main/src/Menu.ts b/packages/main/src/Menu.ts index 3c65fc0b7fee..624bed919d58 100644 --- a/packages/main/src/Menu.ts +++ b/packages/main/src/Menu.ts @@ -25,8 +25,8 @@ import type { Timeout } from "@ui5/webcomponents-base/dist/types.js"; import { renderFinished } from "@ui5/webcomponents-base/dist/Render.js"; import DOMReferenceConverter from "@ui5/webcomponents-base/dist/converters/DOMReference.js"; import type ResponsivePopover from "./ResponsivePopover.js"; -import type MenuItemT from "./MenuItem.js"; -import * as MenuItem from "./MenuItem.js"; +import type MenuItem from "./MenuItem.js"; +import { isInstanceOfMenuItem } from "./MenuItem.js"; import type PopoverHorizontalAlign from "./types/PopoverHorizontalAlign.js"; import "./MenuSeparator.js"; import type { @@ -54,11 +54,11 @@ interface IMenuItem extends UI5Element { } type MenuItemClickEventDetail = { - item: MenuItemT, + item: MenuItem, text: string, } -type MenuBeforeOpenEventDetail = { item?: MenuItemT }; +type MenuBeforeOpenEventDetail = { item?: MenuItem }; type MenuBeforeCloseEventDetail = { escPressed: boolean }; /** @@ -261,7 +261,7 @@ class Menu extends UI5Element { } get _menuItems() { - return this.items.filter((item): item is MenuItemT => !item.isSeparator); + return this.items.filter((item): item is MenuItem => !item.isSeparator); } get acessibleNameText() { @@ -280,7 +280,7 @@ class Menu extends UI5Element { this.open = false; } - _openItemSubMenu(item: MenuItemT) { + _openItemSubMenu(item: MenuItem) { clearTimeout(this._timeout); if (!item._popover || item._popover.open) { @@ -300,8 +300,8 @@ class Menu extends UI5Element { return; } - const item = e.target as MenuItemT; - if (!MenuItem.isInstanceOfMenuItem(item)) { + const item = e.target as MenuItem; + if (!isInstanceOfMenuItem(item)) { return; } @@ -322,7 +322,7 @@ class Menu extends UI5Element { return super.focus(focusOptions); } - _closeOtherSubMenus(item: MenuItemT) { + _closeOtherSubMenus(item: MenuItem) { const menuItems = this._menuItems; if (!menuItems.includes(item)) { return; @@ -335,7 +335,7 @@ class Menu extends UI5Element { }); } - _startOpenTimeout(item: MenuItemT) { + _startOpenTimeout(item: MenuItem) { clearTimeout(this._timeout); this._timeout = setTimeout(() => { @@ -346,7 +346,7 @@ class Menu extends UI5Element { } _itemClick(e: CustomEvent) { - const item = e.detail.item as MenuItemT; + const item = e.detail.item as MenuItem; if (!item._popover) { const prevented = !this.fireDecoratorEvent("item-click", { @@ -364,9 +364,9 @@ class Menu extends UI5Element { _itemKeyDown(e: KeyboardEvent) { const isTabNextPrevious = isTabNext(e) || isTabPrevious(e); - const item = e.target as MenuItemT; + const item = e.target as MenuItem; - if (!MenuItem.isInstanceOfMenuItem(item)) { + if (!isInstanceOfMenuItem(item)) { return; } @@ -393,7 +393,7 @@ class Menu extends UI5Element { } _navigateOutOfEndContent(e: CustomEvent) { - const item = e.target as MenuItemT; + const item = e.target as MenuItem; const shouldNavigateToNextItem = e.detail.shouldNavigateToNextItem; const menuItems = this._menuItems; const itemIndex = menuItems.indexOf(item); From cbd8e40372f01670a571a85fa96b46a96f20ba65 Mon Sep 17 00:00:00 2001 From: Diana Petcheva Date: Mon, 12 May 2025 13:43:11 +0300 Subject: [PATCH 11/11] chore: add comment to prevent accidental removal --- packages/main/src/Menu.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/main/src/Menu.ts b/packages/main/src/Menu.ts index 624bed919d58..c7ac7a46ef01 100644 --- a/packages/main/src/Menu.ts +++ b/packages/main/src/Menu.ts @@ -26,6 +26,7 @@ import { renderFinished } from "@ui5/webcomponents-base/dist/Render.js"; import DOMReferenceConverter from "@ui5/webcomponents-base/dist/converters/DOMReference.js"; import type ResponsivePopover from "./ResponsivePopover.js"; import type MenuItem from "./MenuItem.js"; +// The import below should be kept, as MenuItem is part of the Menu component. import { isInstanceOfMenuItem } from "./MenuItem.js"; import type PopoverHorizontalAlign from "./types/PopoverHorizontalAlign.js"; import "./MenuSeparator.js";