MudMenuItem: Add OnAction for either click or touch but invoked only once by GRMagic · Pull Request #7651 · MudBlazor/MudBlazor

GRMagic · 2023-10-16T12:27:04Z

Description

OnAction will only be invoked once, when the user clicks or touch the menu item.
If OnClick is set, OnAction is not called on user clicks.
If OnTouch is set, OnAction is not called on user touch.
Thus maintaining backwards compatibility.

Example:
<MudMenuItem OnAction="MyAction">My Item</MudMenuItem>

How Has This Been Tested?

Tests in MenuItemActionTest.cs
I don't have an Apple, could someone test it for me?

Types 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)

Checklist:

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

See

Discussion: #6954

codecov · 2023-10-16T12:31:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ab16dbd) 90.57% compared to head (d450f4e) 86.19%.
Report is 7 commits behind head on dev.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #7651      +/-   ##
==========================================
- Coverage   90.57%   86.19%   -4.39%     
==========================================
  Files         427      427              
  Lines       15223    15259      +36     
  Branches        0     3326    +3326     
==========================================
- Hits        13788    13152     -636     
+ Misses       1435     1424      -11     
- Partials        0      683     +683

Files	Coverage Δ
src/MudBlazor/Components/Menu/MudMenuItem.razor.cs	`78.94% <100.00%> (-3.67%)`	⬇️
src/MudBlazor/Services/MudGlobal.cs	`50.00% <100.00%> (+16.66%)`	⬆️

... and 194 files with indirect coverage changes

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

src/MudBlazor/Components/Menu/MudMenuItem.razor.cs

src/MudBlazor.UnitTests.Viewer/TestComponents/Menu/MenuItemActionTest.razor

src/MudBlazor/Components/Menu/MudMenuItem.razor.cs

ScarletKuro · 2023-10-16T18:27:22Z

I don't have an Apple, could someone test it for me?

On what platforms did you test it? Browser(wasm, bss), android(maui), windows(maui)?

GRMagic · 2023-10-17T11:19:28Z

browser (wasm) on Windows and Android

- _lastActionHandled from protected to private.

… calls.

GRMagic · 2023-10-18T14:05:21Z

The page https://mudblazor.com/api/menuitem exists, but I didn't find it in the documentation. Is this intentional?

ScarletKuro · 2023-10-18T14:31:12Z

The page https://mudblazor.com/api/menuitem exists, but I didn't find it in the documentation. Is this intentional?

It's under menu https://mudblazor.com/components/menu because MudMenuItem can only exist inside the Menu.

ScarletKuro · 2023-10-20T13:39:25Z

Before merging, we would need someone to test this on iOS, sadly I don't have Iphone either.
@henon do we have any core member who does?

Also regarding this

I wanted to prevent two simultaneous tasks (OnClick and OnTouch) from accessing the _lastActionHandled variable at a time when it was different from ev, this would cause the event to be fired twice.

If you want to do that, I would consider using the SemaphoreSlim(1, 1) instead. Using "lock" or Monitor can cause deadlocks in some scenarios. Yes, in this particular case it's actually safe, but I guess I still would prefer to have the most safest and correct code by using SemaphoreSlim in case if something changes in future.

henon · 2023-10-21T14:56:38Z

src/MudBlazor/Components/Menu/MudMenuItem.razor.cs

+        /// </remarks>
+        [Parameter]
+        [Category(CategoryTypes.Menu.Behavior)]
+        public TimeSpan SilenceTime { get; set; } = TimeSpan.FromMilliseconds(100);


I think we should call this DebounceInterval to be consistent with other components. Also the term debounce is fitting here, see: https://www.freecodecamp.org/news/javascript-debounce-example/#:~:text=What%20is%20debounce%3F,your%20%E2%80%9Cclick%E2%80%9D%20multiple%20times.

Actually this is something you usually want to change for every menu in the application if you want to change it. I suspect this will never ever have to be changed anyway.

@ScarletKuro What would you say about not making it a [Parameter] and instead public static TimeSpan DebounceInterval { get; set; } = TimeSpan.FromMilliseconds(100); ?

I think it makes sense, because if it is necessary to change this value for one component, it will probably be necessary to change it for all of the items. Can I change it to static?

Yes, make it static and remove the attributes, which are then no longer needed.

@henon wouldn't it make sense to add it to the MudGlobal instead?

You are right, I have already forgotten about that. MenuItemDebounceInterval ?

So, I put it as static property in static class MudGlobal with name MenuItemDebounceInterval?

henon · 2023-10-24T14:52:04Z

IOS is untested at this moment, nobody in the team is IPhone user. We'll just have to risk it.

henon · 2023-10-24T14:55:14Z

Thanks @GRMagic !

…once (MudBlazor#7651) * MudMenuItem Href, OnClick, OnTouch, OnAction * MudGlobal.MenuItemDebounceInterval

dennisrahmen · 2023-11-25T22:18:04Z

@GRMagic thanks for this improvement, I just tested it. It seems to not be working correctly on mobile.
With my menu items, I trigger a function that opens a confirm dialog, but after switching to 'OnAction' it briefly opens and then closes again. Particular noticeable when I hard refresh the page then the first view tries the popup does not stay open.

So there is something happening there, since it is working fine in Desktop mode as well as with 'OnClick' and 'OnTouch'

GRMagic · 2023-11-26T01:40:32Z

Did you remove onclick and ontouch, leaving only onaction?

dennisrahmen · 2023-11-27T08:02:01Z

@GRMagic yes, I only have OnAction.

You can check it here: https://try.mudblazor.com/snippet/QEwxvlGVwWrSSpbr
OnClick works in desktop and OnTouch works in phone mode but OnAction does not work at all in desktop mode and has the bug I described earlier in phone mode.

dennisrahmen · 2023-11-27T09:37:00Z

Maybe something specific to WASM? As I use it and tryMudBlazor also uses WASM.

GRMagic · 2023-11-27T09:45:37Z

I am also using wasm. I tested your code on my development branch and it worked.
I'll test it on the mudblazor master...

GRMagic · 2023-11-27T09:52:03Z

In master the code also works, but in try mudblazor it didn't work.

GRMagic · 2023-11-27T10:01:24Z

Upgraded a wasm project to mudblazor version 6.11.1, and tested your code on it, it worked.
I noticed that in the bottom right corner of trymudblazor, it shows version 6.11.0.
Which version are you using in your project? Could you update to version 6.11.1 and check if the problem still occurs?

dennisrahmen · 2023-11-27T10:06:04Z

I am using 6.11.1 in my App.
I will try clearing the cache rebuilding the projekt maybe that helps.

GRMagic · 2023-11-27T10:08:52Z

In most of the time it is preferable to use async Task instead of async void. Try change it too.

dennisrahmen · 2023-11-27T12:25:25Z

I just pulled the code in the TryMud together from two examples to make it simpler:
https://mudblazor.com/components/messagebox#message-box
https://mudblazor.com/components/menu#customization

This is my code inside a DataGrid column:

<TemplateColumn Sortable="false" Filterable="false">
    <CellTemplate>
        <MudMenu Dense="true" Icon="@Icons.Material.Filled.MoreVert" Size="Size.Small" Color="Color.Primary">
            <MudMenuItem IconSize="Size.Small" IconColor="Color.Warning" Icon="@Icons.Material.Filled.Edit" OnAction="() => UpdateContact(context.Item)">Bearbeiten</MudMenuItem>
            <MudDivider Light DividerType="DividerType.Middle" Class="my-2"/>
            <MudMenuItem IconSize="Size.Small" IconColor="Color.Error" Icon="@Icons.Material.Filled.Delete" OnAction="() => RemoveContact(context.Item)">Löschen</MudMenuItem>
        </MudMenu>
    </CellTemplate>
</TemplateColumn>

And the code behind:

private async Task RemoveContact(ContactDTO contactDTO)
{
    var result = await DialogService.ShowMessageBox("Löschen bestätigen", $"Das löschen von '{contactDTO.FullName}' kann nicht rückgängig gemacht werden!", "Löschen!", cancelText: "Abbrechen");
    if (result == true)
    {
        HttpResponseDTO resp = await HttpService.Delete("Contact/", contactDTO.Id.ToString());
        if (resp.LoadingSuccessful)
        {
            GetData();
            Snackbar.Add("Kontakt gelöscht", Severity.Success);
        }
        else
        {
            Snackbar.Add("Fehler beim löschen des Kontakts", Severity.Error);
        }
    }
}

MudMenuItem Href, OnClick, OnTouch, OnAction

537a5cd

github-actions bot added enhancement Adds a new feature or enhances existing functionality (not fixing a defect) in the main library PR: needs review labels Oct 16, 2023

ScarletKuro reviewed Oct 16, 2023

View reviewed changes

GRMagic added 4 commits October 17, 2023 08:22

- from OnActionHandler to OnActionHandlerAsync.

6df3293

- _lastActionHandled from protected to private.

Test event OnAction on MudMenuItem

3361872

Ugly workarroud for Android OnClick +OnTouch

a61dcf3

Parameter that allow user to define the minimum time between OnAction…

8a3c0d4

… calls.

GRMagic requested a review from ScarletKuro October 20, 2023 10:31

ScarletKuro requested a review from henon October 20, 2023 13:30

henon reviewed Oct 21, 2023

View reviewed changes

GRMagic added 2 commits October 24, 2023 08:55

Debounce approach

912e3c1

MudGlobal.MenuItemDebounceInterval

d450f4e

henon changed the title ~~MudMenuItem OnAction~~ MudMenuItem: Add OnAction for either click or touch but invoked only once Oct 24, 2023

henon merged commit 4293b1a into MudBlazor:dev Oct 24, 2023

henon removed the PR: needs review label Oct 24, 2023

ilovepilav pushed a commit to ilovepilav/MudBlazor that referenced this pull request Nov 25, 2023

MudMenuItem: Add OnAction for either click or touch but invoked only …

d0052c2

…once (MudBlazor#7651) * MudMenuItem Href, OnClick, OnTouch, OnAction * MudGlobal.MenuItemDebounceInterval

ScarletKuro mentioned this pull request Mar 26, 2024

Menu: Improve touch support & Merge OnTouch,OnAction into OnClick #8492

Merged

6 tasks

Uh oh!

Conversation

GRMagic commented Oct 16, 2023

Description

How Has This Been Tested?

Types of changes

Checklist:

See

Uh oh!

codecov bot commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ScarletKuro commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GRMagic commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GRMagic commented Oct 18, 2023

Uh oh!

ScarletKuro commented Oct 18, 2023

Uh oh!

ScarletKuro commented Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henon Oct 21, 2023

Choose a reason for hiding this comment

Uh oh!

GRMagic Oct 24, 2023

Choose a reason for hiding this comment

Uh oh!

henon Oct 24, 2023

Choose a reason for hiding this comment

Uh oh!

ScarletKuro Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henon Oct 24, 2023

Choose a reason for hiding this comment

Uh oh!

GRMagic Oct 24, 2023

Choose a reason for hiding this comment

Uh oh!

henon Oct 24, 2023

Choose a reason for hiding this comment

Uh oh!

henon commented Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henon commented Oct 24, 2023

Uh oh!

dennisrahmen commented Nov 25, 2023

Uh oh!

GRMagic commented Nov 26, 2023

Uh oh!

dennisrahmen commented Nov 27, 2023

Uh oh!

dennisrahmen commented Nov 27, 2023

Uh oh!

GRMagic commented Nov 27, 2023

Uh oh!

GRMagic commented Nov 27, 2023

Uh oh!

GRMagic commented Nov 27, 2023

Uh oh!

dennisrahmen commented Nov 27, 2023

Uh oh!

GRMagic commented Nov 27, 2023

Uh oh!

dennisrahmen commented Nov 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

codecov bot commented Oct 16, 2023 •

edited

Loading

ScarletKuro commented Oct 16, 2023 •

edited

Loading

GRMagic commented Oct 17, 2023 •

edited

Loading

ScarletKuro commented Oct 20, 2023 •

edited

Loading

ScarletKuro Oct 24, 2023 •

edited

Loading

henon commented Oct 24, 2023 •

edited

Loading

dennisrahmen commented Nov 27, 2023 •

edited

Loading