SubMenu: Fix expanding sub menu items on touch devices #93208

yincongcyincong · 2024-09-11T09:43:51Z

Fixes #93151
actually, i add mobile click event onTouchStart, and use event delegation to avoid menu is closed
please take a look @tskarhed @torkelo
https://github.com/user-attachments/assets/4ce7541c-7c5d-4d35-93bd-18ff6e960829

ashharrison90

thanks for raising the issue and looking at this! 🙌

think maybe a better approach would be to change the onClick in MenuItem to be something like:

        onClick={(event) => {
          if (hasSubMenu) {
            event.preventDefault();
            event.stopPropagation();
          }
          onClick?.(event);
        }}

that should stop the click event propagating up to the overlay and closing the menu.

note: this isn't perfect, if the new submenu overflows the bounds of the mobile view it will resize the whole window. but it at least unblocks people from using these submenus. i think the changes needed to properly support mobile behaviour for the Menu and MenuItem elements are bigger than can be delivered in a community PR 😅

ashharrison90 · 2024-09-13T15:49:32Z

packages/grafana-ui/src/components/Dropdown/Dropdown.tsx

+  const onOverlayClicked = (event: React.MouseEvent<HTMLDivElement, MouseEvent>) => {
+    if (event.target instanceof HTMLDivElement || event.target instanceof HTMLSpanElement) {
+      const innerText = event.target.innerText
+      if (innerText.includes("More...")) {
+        return
+      }
+    }
+


this is definitely too brittle i'm afraid 😬

thanks for your review, bro. i will modify it.

yincongcyincong · 2024-09-17T07:19:55Z

@ashharrison90 could you please look this pr again?

tskarhed

This doesn't seem to quite work. We want all the use cases for the menu to support touch for submenu, so it is better if the stopPropagation is used in MenuItem. Try the approach that @ashharrison90 suggested!

yincongcyincong · 2024-09-24T10:49:25Z

This doesn't seem to quite work. We want all the use cases for the menu to support touch for submenu, so it is better if the stopPropagation is used in MenuItem. Try the approach that @ashharrison90 suggested!

thanks for review. i have add stopPropagation at MenuItem. please take a look at lastest pr.
https://github.com/grafana/grafana/pull/93208/files#diff-6274d9463d0536b679c48ba4551dbc84354b1a742e5d385f01d61fa494f27d75R78

tskarhed · 2024-09-24T11:34:26Z

thanks for review. i have add stopPropagation at MenuItem. please take a look at lastest pr. https://github.com/grafana/grafana/pull/93208/files#diff-6274d9463d0536b679c48ba4551dbc84354b1a742e5d385f01d61fa494f27d75R78

The issue is still there:
https://github.com/user-attachments/assets/158db928-5672-42db-b590-d61e2bdda399

You have added stopPropagation only for onMore, which doesn't account for inspect or any other use case.

Try adding the ash's requested changes here:

grafana/packages/grafana-ui/src/components/Menu/MenuItem.tsx

Line 155 in 5459d49

onClick={onClick}

yincongcyincong · 2024-09-24T12:03:06Z

thanks for review. i have add stopPropagation at MenuItem. please take a look at lastest pr. https://github.com/grafana/grafana/pull/93208/files#diff-6274d9463d0536b679c48ba4551dbc84354b1a742e5d385f01d61fa494f27d75R78

The issue is still there: https://github.com/user-attachments/assets/158db928-5672-42db-b590-d61e2bdda399

You have added stopPropagation only for onMore, which doesn't account for inspect or any other use case.

Try adding the ash's requested changes here:

grafana/packages/grafana-ui/src/components/Menu/MenuItem.tsx

Line 155 in 5459d49

onClick={onClick}

yep. actually this pr don't work on inspect. because inspect is very special. itself has a click event. if i modify the click event. it will result some other bug. like this.

tskarhed · 2024-09-25T07:35:17Z

yep. actually this pr don't work on inspect. because inspect is very special. itself has a click event. if i modify the click event. it will result some other bug. like this.

You're right. This functionality doesn't make sense. Not calling onClick when there is a submenu is more consistent behavior. Can you add those changes too?

yincongcyincong · 2024-09-25T11:06:04Z

yep. actually this pr don't work on inspect. because inspect is very special. itself has a click event. if i modify the click event. it will result some other bug. like this.

You're right. This functionality doesn't make sense. Not calling onClick when there is a submenu is more consistent behavior. Can you add those changes too?

i'm pleasure contribute to grafana. i will give a new pr!

tskarhed · 2024-09-25T11:22:55Z

i'm pleasure contribute to grafana. i will give a new pr!

It is okay if you do it in this PR, as it will make it easier to fix the other issue :)

yincongcyincong · 2024-09-25T11:33:03Z

i'm pleasure contribute to grafana. i will give a new pr!

It is okay if you do it in this PR, as it will make it easier to fix the other issue :)

ok, i will change this pr. thanks for your review.

yincongcyincong · 2024-09-26T03:36:43Z

@tskarhed bro, i delete inspect click function and add stopPropagation at menu that have submenu. please take a look again. thanks

video-1727321713273.mp4

tskarhed · 2024-09-30T10:13:17Z

packages/grafana-ui/src/components/Menu/MenuItem.tsx

@@ -108,7 +108,8 @@ export const MenuItem = React.memo(
        [styles.disabled]: disabled,
        [styles.destructive]: destructive && !disabled,
      },
-      className
+      className,
+      hasSubMenu ? 'hasSubmenu' : 'noSubmenu'


Instead of using a class as a selector, you could add aria-haspopup="menu" to the item that has a submenu and use that when selecting closest.

ashharrison90

think the changes in Menu/MenuItem can simplify down into just modifying the onClick - see suggestions. i think this handles all cases 🙏 thanks again for tackling this 🙇

ashharrison90 · 2024-10-04T14:29:25Z

packages/grafana-ui/src/components/Menu/MenuItem.tsx

        onMouseEnter={onMouseEnter}
        onMouseLeave={onMouseLeave}
        onKeyDown={handleKeys}
+        onTouchStart={onMouseEnter}


Suggested change

onTouchStart={onMouseEnter}

ashharrison90 · 2024-10-04T14:29:32Z

packages/grafana-ui/src/components/Menu/MenuItem.tsx

@@ -108,7 +108,8 @@ export const MenuItem = React.memo(
        [styles.disabled]: disabled,
        [styles.destructive]: destructive && !disabled,
      },
-      className
+      className,
+      hasSubMenu ? 'hasSubmenu' : 'noSubmenu'


Suggested change

hasSubMenu ? 'hasSubmenu' : 'noSubmenu'

ashharrison90 · 2024-10-04T14:29:49Z

packages/grafana-ui/src/components/Menu/MenuItem.tsx

+        onClick={(event) => {
+          if (hasSubMenu && !(event.target as HTMLElement).closest('.noSubmenu')) {
+            event.preventDefault();
+            event.stopPropagation();
+          }
+          onClick?.(event);
+        }}


Suggested change

onClick={(event) => {

if (hasSubMenu && !(event.target as HTMLElement).closest('.noSubmenu')) {

event.preventDefault();

event.stopPropagation();

}

onClick?.(event);

}}

onClick={(event) => {

if (hasSubMenu && !isSubMenuOpen) {

event.preventDefault();

event.stopPropagation();

}

onClick?.(event);

}}

yincongcyincong · 2024-10-07T02:44:39Z

@tskarhed @ashharrison90 bros, thanks for your review. i gived a new pr.

ashharrison90

think this looks great 👍 thanks for the contribution! 🙌

click more in mobile phone

1a5ace5

yincongcyincong requested a review from a team as a code owner September 11, 2024 09:43

yincongcyincong requested review from Clarity-89 and JoaoSilvaGrafana and removed request for a team September 11, 2024 09:43

github-actions bot added this to the 11.3.x milestone Sep 11, 2024

grafana-pr-automation bot added area/frontend pr/external This PR is from external contributor labels Sep 11, 2024

ashharrison90 requested changes Sep 13, 2024

View reviewed changes

use stopPropagation stop event

61a2155

yincongcyincong requested a review from a team as a code owner September 14, 2024 02:00

yincongcyincong requested review from axelavargas and mdvictor and removed request for a team September 14, 2024 02:00

delete log

5459d49

yincongcyincong requested a review from ashharrison90 September 19, 2024 12:49

tskarhed requested changes Sep 24, 2024

View reviewed changes

delete inspect click and add stopPropagation

0599349

tskarhed reviewed Sep 30, 2024

View reviewed changes

ashharrison90 requested changes Oct 4, 2024

View reviewed changes

bug fix

08180c4

ashharrison90 added add to changelog no-backport Skip backport of PR labels Oct 7, 2024

ashharrison90 changed the title ~~fix: clicking more in mobile phone don't work~~ SubMenu: Fix expanding sub menu items on touch devices Oct 7, 2024

ashharrison90 approved these changes Oct 7, 2024

View reviewed changes

ashharrison90 added 2 commits October 7, 2024 11:46

fix unit tests

2a3466d

Merge branch 'main' into click_more_in_mobile

cf32865

tskarhed approved these changes Oct 7, 2024

View reviewed changes

ashharrison90 added 2 commits October 7, 2024 14:03

fix e2e tests

1b21180

fix old-arch tests

db10808

ashharrison90 merged commit 63bdbb6 into grafana:main Oct 7, 2024
16 of 17 checks passed

joshhunt modified the milestones: 11.3.x, 11.3.0 Oct 18, 2024

sujeet mentioned this pull request Feb 8, 2025

Enforce UID uniqueness in browse folder UI oodle-ai/grafana#47

Merged

sujeet mentioned this pull request Feb 28, 2025

Redirect Grafana's alerts pages to Oodle's oodle-ai/grafana#53

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SubMenu: Fix expanding sub menu items on touch devices #93208

SubMenu: Fix expanding sub menu items on touch devices #93208

yincongcyincong commented Sep 11, 2024

ashharrison90 left a comment

ashharrison90 Sep 13, 2024

yincongcyincong Sep 14, 2024

yincongcyincong commented Sep 17, 2024

tskarhed left a comment

yincongcyincong commented Sep 24, 2024 •

edited

Loading

tskarhed commented Sep 24, 2024

yincongcyincong commented Sep 24, 2024

tskarhed commented Sep 25, 2024

yincongcyincong commented Sep 25, 2024

tskarhed commented Sep 25, 2024

yincongcyincong commented Sep 25, 2024

yincongcyincong commented Sep 26, 2024

tskarhed Sep 30, 2024

ashharrison90 left a comment

ashharrison90 Oct 4, 2024

ashharrison90 Oct 4, 2024

ashharrison90 Oct 4, 2024

yincongcyincong commented Oct 7, 2024

ashharrison90 left a comment

SubMenu: Fix expanding sub menu items on touch devices #93208

SubMenu: Fix expanding sub menu items on touch devices #93208

Conversation

yincongcyincong commented Sep 11, 2024

ashharrison90 left a comment

Choose a reason for hiding this comment

ashharrison90 Sep 13, 2024

Choose a reason for hiding this comment

yincongcyincong Sep 14, 2024

Choose a reason for hiding this comment

yincongcyincong commented Sep 17, 2024

tskarhed left a comment

Choose a reason for hiding this comment

yincongcyincong commented Sep 24, 2024 • edited Loading

tskarhed commented Sep 24, 2024

yincongcyincong commented Sep 24, 2024

tskarhed commented Sep 25, 2024

yincongcyincong commented Sep 25, 2024

tskarhed commented Sep 25, 2024

yincongcyincong commented Sep 25, 2024

yincongcyincong commented Sep 26, 2024

tskarhed Sep 30, 2024

Choose a reason for hiding this comment

ashharrison90 left a comment

Choose a reason for hiding this comment

ashharrison90 Oct 4, 2024

Choose a reason for hiding this comment

ashharrison90 Oct 4, 2024

Choose a reason for hiding this comment

ashharrison90 Oct 4, 2024

Choose a reason for hiding this comment

yincongcyincong commented Oct 7, 2024

ashharrison90 left a comment

Choose a reason for hiding this comment

yincongcyincong commented Sep 24, 2024 •

edited

Loading