Skip to content

MudMenu: Fix nested menu to support multiple levels. (#7376)#9537

Merged
henon merged 9 commits intoMudBlazor:devfrom
charles7668:fix/MudMenu-nest-with-MouseOver
Oct 18, 2024
Merged

MudMenu: Fix nested menu to support multiple levels. (#7376)#9537
henon merged 9 commits intoMudBlazor:devfrom
charles7668:fix/MudMenu-nest-with-MouseOver

Conversation

@charles7668
Copy link
Contributor

@charles7668 charles7668 commented Jul 31, 2024

Description

When nesting menus more than one level deep with mouse over, the parent menu cannot stay displayed when the mouse moves to the nested menu.

close #7376

How Has This Been Tested?

I add a test page in MudBlazor.UnitTests.Viewer

Type of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (fix or improvement to the website or code docs)

before:
before

after:
after

Checklist

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

@github-actions github-actions bot added bug Unexpected behavior or functionality not working as intended PR: needs review labels Jul 31, 2024
@codecov
Copy link

codecov bot commented Jul 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.57%. Comparing base (28bc599) to head (2f37170).
Report is 590 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #9537      +/-   ##
==========================================
+ Coverage   89.82%   90.57%   +0.74%     
==========================================
  Files         412      406       -6     
  Lines       11878    12749     +871     
  Branches     2364     2473     +109     
==========================================
+ Hits        10670    11547     +877     
+ Misses        681      641      -40     
- Partials      527      561      +34     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ScarletKuro
Copy link
Member

@danielchalmers duplicate of PR #9513?

@ScarletKuro
Copy link
Member

I add a test page in MudBlazor.UnitTests.Viewer

I don't see that this page was used anywhere on the bUnit side, meaning it's not doing anything.

@charles7668 charles7668 force-pushed the fix/MudMenu-nest-with-MouseOver branch from 61de7f2 to 306eb69 Compare July 31, 2024 14:30
@charles7668 charles7668 force-pushed the fix/MudMenu-nest-with-MouseOver branch from 306eb69 to 004aeb9 Compare July 31, 2024 14:38
@charles7668
Copy link
Contributor Author

@danielchalmers duplicate of PR #9513?

I think this PR is a different fix. Before I started this fix, I checked this PR, and it doesn't seem to address the nested menu issue.

I add a test page in MudBlazor.UnitTests.Viewer

I don't see that this page was used anywhere on the bUnit side, meaning it's not doing anything.

Updated tests.

@danielchalmers
Copy link
Member

Hi, the nested menus are staying open

video4.mp4

@charles7668
Copy link
Contributor Author

Hi, the nested menus are staying open
video4.mp4

Updated

firefox_QiWM2Cb86S

@danielchalmers
Copy link
Member

Branch has conflicts after merging the other PR

@charles7668 charles7668 force-pushed the fix/MudMenu-nest-with-MouseOver branch from 3c8b1e9 to 4598e04 Compare August 8, 2024 01:07
@charles7668
Copy link
Contributor Author

Branch has conflicts after merging the other PR

Updated

@ScarletKuro
Copy link
Member

ScarletKuro commented Aug 8, 2024

@danielchalmers @henon what about do, is it more readable?

var menu = this;
do
{
	menu._isPointerOver = false;
	menu = menu.ParentMenu;
} while (menu?.ActivationEvent == MouseEvent.MouseOver);

// If this menu is temporary and activated by MouseOver, handle delayed closure.
if (_isTemporary && ActivationEvent == MouseEvent.MouseOver)
{
	// Allow a brief delay to check if the pointer re-enters the menu.
	await Task.Delay(100);

	// Close the menu if the pointer hasn't re-entered and the menu is still temporary.
	menu = this;
	do
	{
		if (!menu._isPointerOver && menu._isTemporary)
		{
			await menu.CloseMenuAsync();
		}
		menu = menu.ParentMenu;
	} while (menu?.ActivationEvent == MouseEvent.MouseOver);
}

Anyway, looks good to me.

@ScarletKuro ScarletKuro requested a review from henon August 8, 2024 14:00
Copy link
Member

@danielchalmers danielchalmers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't clicking an item close the whole tree?

video4.mp4

Could update the Nested Menu example so it's MouseOver instead of a weird hybrid
 
https://github.com/user-attachments/assets/503afeeb-3b94-40e5-bffd-fa2d2911fc58

@charles7668 charles7668 marked this pull request as draft August 10, 2024 06:23
@charles7668 charles7668 force-pushed the fix/MudMenu-nest-with-MouseOver branch from 92bd9d7 to 2f37170 Compare August 10, 2024 10:48
@charles7668 charles7668 marked this pull request as ready for review August 10, 2024 10:59
@charles7668
Copy link
Contributor Author

updated

firefox_CfON6lObNu

@henon
Copy link
Contributor

henon commented Aug 21, 2024

what about do, is it more readable?

In general you choose do over while if you always want to have at least one iteration of the loop before the condition is checked. If that is what the author wanted to do in this case then do is the correct choice otherwise I'd suggest changing to while

@ScarletKuro
Copy link
Member

Lets merge this? We don't have a better design on how to fix this anyway

@henon henon merged commit cad40b7 into MudBlazor:dev Oct 18, 2024
@henon
Copy link
Contributor

henon commented Oct 18, 2024

Thanks @charles7668

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Unexpected behavior or functionality not working as intended

Projects

None yet

4 participants