From 63bdbb67fce896f848257b5d989e6cc56e0ffed6 Mon Sep 17 00:00:00 2001 From: jackyin Date: Mon, 7 Oct 2024 22:22:18 +0800 Subject: [PATCH] SubMenu: Fix expanding sub menu items on touch devices (#93208) * click more in mobile phone * use stopPropagation stop event * delete log * delete inspect click and add stopPropagation * bug fix * fix unit tests * fix e2e tests * fix old-arch tests --------- Co-authored-by: Ashley Harrison --- e2e/old-arch/utils/flows/importDashboard.ts | 3 ++- .../various-suite/inspect-drawer.spec.ts | 6 ++++-- e2e/utils/flows/importDashboard.ts | 3 ++- e2e/various-suite/inspect-drawer.spec.ts | 6 ++++-- .../grafana-ui/src/components/Menu/MenuItem.tsx | 8 +++++++- .../pages/DashboardScenePage.test.tsx | 4 ++-- .../dashboard/utils/getPanelMenu.test.ts | 4 ---- .../app/features/dashboard/utils/getPanelMenu.ts | 16 ---------------- 8 files changed, 21 insertions(+), 29 deletions(-) diff --git a/e2e/old-arch/utils/flows/importDashboard.ts b/e2e/old-arch/utils/flows/importDashboard.ts index 4e7b8ade4ad..7caadac21dd 100644 --- a/e2e/old-arch/utils/flows/importDashboard.ts +++ b/e2e/old-arch/utils/flows/importDashboard.ts @@ -49,7 +49,8 @@ export const importDashboard = (dashboardToImport: Dashboard, queryTimeout?: num dashboardToImport.panels.forEach((panel) => { // Look at the json data e2e.components.Panels.Panel.menu(panel.title).click({ force: true }); // force click because menu is hidden and show on hover - e2e.components.Panels.Panel.menuItems('Inspect').should('be.visible').click(); + e2e.components.Panels.Panel.menuItems('Inspect').trigger('mouseover', { force: true }); + e2e.components.Panels.Panel.menuItems('Data').click({ force: true }); e2e.components.Tab.title('JSON').should('be.visible').click(); e2e.components.PanelInspector.Json.content().should('be.visible').contains('Panel JSON').click({ force: true }); e2e.components.Select.option().should('be.visible').contains('Panel data').click(); diff --git a/e2e/old-arch/various-suite/inspect-drawer.spec.ts b/e2e/old-arch/various-suite/inspect-drawer.spec.ts index 2faf952ba08..48746dc7a9f 100644 --- a/e2e/old-arch/various-suite/inspect-drawer.spec.ts +++ b/e2e/old-arch/various-suite/inspect-drawer.spec.ts @@ -34,8 +34,10 @@ describe('Inspect drawer tests', () => { e2e.flows.openDashboard({ uid: 'wfTJJL5Wz' }); - // testing opening inspect drawer directly by clicking on Inspect in header menu - e2e.flows.openPanelMenuItem(e2e.flows.PanelMenuItems.Inspect, PANEL_UNDER_TEST); + e2e.components.Panels.Panel.title(PANEL_UNDER_TEST).scrollIntoView().should('be.visible'); + e2e.components.Panels.Panel.menu(PANEL_UNDER_TEST).click({ force: true }); // force click because menu is hidden and show on hover + e2e.components.Panels.Panel.menuItems('Inspect').trigger('mouseover', { force: true }); + e2e.components.Panels.Panel.menuItems('Data').click({ force: true }); expectDrawerTabsAndContent(); diff --git a/e2e/utils/flows/importDashboard.ts b/e2e/utils/flows/importDashboard.ts index d3c6b7e23b5..7b5e04fd0b1 100644 --- a/e2e/utils/flows/importDashboard.ts +++ b/e2e/utils/flows/importDashboard.ts @@ -49,7 +49,8 @@ export const importDashboard = (dashboardToImport: Dashboard, queryTimeout?: num dashboardToImport.panels.forEach((panel) => { // Look at the json data e2e.components.Panels.Panel.menu(panel.title).click({ force: true }); // force click because menu is hidden and show on hover - e2e.components.Panels.Panel.menuItems('Inspect').should('be.visible').click(); + e2e.components.Panels.Panel.menuItems('Inspect').trigger('mouseover', { force: true }); + e2e.components.Panels.Panel.menuItems('Data').click({ force: true }); e2e.components.Tab.title('JSON').should('be.visible').click(); e2e.components.PanelInspector.Json.content().should('be.visible'); e2e.components.ReactMonacoEditor.editorLazy().should('be.visible'); diff --git a/e2e/various-suite/inspect-drawer.spec.ts b/e2e/various-suite/inspect-drawer.spec.ts index 2faf952ba08..48746dc7a9f 100644 --- a/e2e/various-suite/inspect-drawer.spec.ts +++ b/e2e/various-suite/inspect-drawer.spec.ts @@ -34,8 +34,10 @@ describe('Inspect drawer tests', () => { e2e.flows.openDashboard({ uid: 'wfTJJL5Wz' }); - // testing opening inspect drawer directly by clicking on Inspect in header menu - e2e.flows.openPanelMenuItem(e2e.flows.PanelMenuItems.Inspect, PANEL_UNDER_TEST); + e2e.components.Panels.Panel.title(PANEL_UNDER_TEST).scrollIntoView().should('be.visible'); + e2e.components.Panels.Panel.menu(PANEL_UNDER_TEST).click({ force: true }); // force click because menu is hidden and show on hover + e2e.components.Panels.Panel.menuItems('Inspect').trigger('mouseover', { force: true }); + e2e.components.Panels.Panel.menuItems('Data').click({ force: true }); expectDrawerTabsAndContent(); diff --git a/packages/grafana-ui/src/components/Menu/MenuItem.tsx b/packages/grafana-ui/src/components/Menu/MenuItem.tsx index 11048b850c8..5bdfb00a682 100644 --- a/packages/grafana-ui/src/components/Menu/MenuItem.tsx +++ b/packages/grafana-ui/src/components/Menu/MenuItem.tsx @@ -152,7 +152,13 @@ export const MenuItem = React.memo( className={itemStyle} rel={target === '_blank' ? 'noopener noreferrer' : undefined} href={url} - onClick={onClick} + onClick={(event) => { + if (hasSubMenu && !isSubMenuOpen) { + event.preventDefault(); + event.stopPropagation(); + } + onClick?.(event); + }} onMouseEnter={onMouseEnter} onMouseLeave={onMouseLeave} onKeyDown={handleKeys} diff --git a/public/app/features/dashboard-scene/pages/DashboardScenePage.test.tsx b/public/app/features/dashboard-scene/pages/DashboardScenePage.test.tsx index fa18446d6b2..54aa8f75151 100644 --- a/public/app/features/dashboard-scene/pages/DashboardScenePage.test.tsx +++ b/public/app/features/dashboard-scene/pages/DashboardScenePage.test.tsx @@ -1,4 +1,4 @@ -import { act, fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { act, render, screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { cloneDeep } from 'lodash'; import { useParams } from 'react-router-dom-v5-compat'; @@ -206,7 +206,7 @@ describe('DashboardScenePage', () => { const inspectMenuItem = await screen.findAllByText('Inspect'); - act(() => fireEvent.click(inspectMenuItem[0])); + await userEvent.click(inspectMenuItem[0]); expect(await screen.findByText('Inspect: Panel B')).toBeInTheDocument(); diff --git a/public/app/features/dashboard/utils/getPanelMenu.test.ts b/public/app/features/dashboard/utils/getPanelMenu.test.ts index 9cb64f51d6d..7827f341781 100644 --- a/public/app/features/dashboard/utils/getPanelMenu.test.ts +++ b/public/app/features/dashboard/utils/getPanelMenu.test.ts @@ -70,7 +70,6 @@ describe('getPanelMenu()', () => { }, { "iconClassName": "info-circle", - "onClick": [Function], "shortcut": "i", "subMenu": [ { @@ -83,7 +82,6 @@ describe('getPanelMenu()', () => { }, { "iconClassName": "cube", - "onClick": [Function], "subMenu": [ { "onClick": [Function], @@ -343,7 +341,6 @@ describe('getPanelMenu()', () => { }, { "iconClassName": "info-circle", - "onClick": [Function], "shortcut": "i", "subMenu": [ { @@ -356,7 +353,6 @@ describe('getPanelMenu()', () => { }, { "iconClassName": "cube", - "onClick": [Function], "subMenu": [ { "href": undefined, diff --git a/public/app/features/dashboard/utils/getPanelMenu.ts b/public/app/features/dashboard/utils/getPanelMenu.ts index 74a6377f2c8..8d485825b16 100644 --- a/public/app/features/dashboard/utils/getPanelMenu.ts +++ b/public/app/features/dashboard/utils/getPanelMenu.ts @@ -73,10 +73,6 @@ export function getPanelMenu( }); }; - const onMore = (event: React.MouseEvent) => { - event.preventDefault(); - }; - const onDuplicatePanel = (event: React.MouseEvent) => { event.preventDefault(); duplicatePanel(dashboard, panel); @@ -176,17 +172,6 @@ export function getPanelMenu( type: 'submenu', text: t('panel.header-menu.inspect', `Inspect`), iconClassName: 'info-circle', - onClick: (e: React.MouseEvent) => { - const currentTarget = e.currentTarget; - const target = e.target; - - if ( - target === currentTarget || - (target instanceof HTMLElement && target.closest('[role="menuitem"]') === currentTarget) - ) { - onInspectPanel(); - } - }, shortcut: 'i', subMenu: inspectMenu, }); @@ -323,7 +308,6 @@ export function getPanelMenu( text: t('panel.header-menu.more', `More...`), iconClassName: 'cube', subMenu, - onClick: onMore, }); }