Snackbar: Allow to attach custom task on close button click #8589

BieleckiLtd · 2024-04-06T10:50:21Z

Allows the attachment of a custom task when a user dismisses the message by clicking the close button.

Description

Closes #7317

How Has This Been Tested?

unit | visually

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.

codecov · 2024-04-06T10:55:59Z

Codecov Report

Attention: Patch coverage is 69.23077% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 89.89%. Comparing base (28bc599) to head (516d06f).
Report is 119 commits behind head on dev.

❗ Current head 516d06f differs from pull request most recent head e8654e4. Consider uploading reports for the commit e8654e4 to get more accurate results

Files	Patch %	Lines
...or/Components/Snackbar/MudSnackbarElement.razor.cs	50.00%	0 Missing and 2 partials ⚠️
src/MudBlazor/Components/Snackbar/Snackbar.cs	71.42%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #8589      +/-   ##
==========================================
+ Coverage   89.82%   89.89%   +0.06%     
==========================================
  Files         412      414       +2     
  Lines       11878    11912      +34     
  Branches     2364     2365       +1     
==========================================
+ Hits        10670    10708      +38     
+ Misses        681      675       -6     
- Partials      527      529       +2

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

danielchalmers

I like the refactors! Interested in seeing how others feel about CodeInline; Makes sense to me.

src/MudBlazor/Components/Snackbar/MudSnackbarElement.razor.cs

src/MudBlazor/Components/Snackbar/Snackbar.cs

danielchalmers · 2024-04-07T03:09:30Z

src/MudBlazor/Components/Snackbar/SnackbarOptions.cs

+        /// <summary>
+        /// The asynchronous delegate that is invoked when the Snackbar is clicked.
+        /// </summary>
+        public Func<Snackbar, Task> OnClick { get; set; }


Love the rename. Needs to be added to the Migration Guide if accepted

OnClickAsync?

src/MudBlazor.Docs/Pages/Components/Snackbar/SnackbarPage.razor

src/MudBlazor.Docs/Pages/Components/Snackbar/Examples/SnackbarOnCloseExample.razor

danielchalmers · 2024-04-09T18:22:30Z

src/MudBlazor/Components/Snackbar/Snackbar.cs


                // Click action is executed only if it's not from the close icon
-                await State.Options.OnClick.Invoke(this);
+                //await State.Options.OnClick.Invoke(this);
+                State.Options.OnClick.Invoke(this);


Hmm, shouldn't Clicked still be async and this call be awaited? I might be totally wrong

ActionClickedAsync and CloseIconClickedAsync could be non-async because they were directly calling another Task

Maybe someone could have a fresh pair of eyes look at it, but with the code as is, my async actions OnClick and OnCloseButtonClick are being awaited correctly in the use case like this:

private void Show() { Snackbar.Add("Close me using the close button.", Severity.Normal, config => { config.RequireInteraction = true; config.OnCloseButtonClick = SayGoodbyeAsync; config.Action = "Adiós"; config.OnClick = SayGoodbyeAsync; }); } public async Task SayGoodbyeAsync(Snackbar snackbar) { await Task.Delay(5000); // this is being awaited ok Snackbar.Add("Sorry to see you go!"); }

The snackbar will be dismissed immediately upon clicking the close button, with any attached task invoked and awaited in the background. I think that would work alright from a UX perspective. On the other hand, if in the Clicked method I await my custom task to complete before closing the snackbar, this could give the impression that the click did not register or the app has hung.

@henon Thoughts on this behavior? Makes sense to me.

Exceptions in the OnClick method would be swallowed though. So the best thing to do would be this:

_ = InvokeAndLogException(State.Options.OnClick); where InvokeAndLogException is:

[Inject] private ILoggerFactory LoggerFactory { get; set; } = null!; private ILogger? _logger; protected ILogger Logger => _logger ??= LoggerFactory.CreateLogger(GetType()); private async Task InvokeAndLogException(Func<Snackbar, Task> func) { try { await func(this) } catch(Exception e) { Logger.LogErrpr("An exception happened in Snackbar.OnClick: {e.Message}\n"); } }

or maybe @ScarletKuro has a better idea

Logger.LogError has an overload that accepts Exception? that might provide stack as well.
Edit: Hm, perhaps not always wanted.

Why it's simply not

await State.Options.OnClick.Invoke(this);

?
It's an Func<Snackbar, Task> and if re are renaming, it probably makes sense to name it OnClickAsync
Otherwise I don't get the Clicked change from void to Task
Well, I mean I understand you did it for the ActionClickedAsync / CloseIconClickedAsync, but it makes sense to use await now when it's a Task.

henon · 2024-05-02T12:15:19Z

This is almost done, isn't it?

ScarletKuro · 2024-05-02T23:40:59Z

src/MudBlazor/Components/Snackbar/Snackbar.cs

+            if (fromCloseIcon)
+            {
+                // Invoke user-defined task when close button is clicked
+                State.Options.OnCloseButtonClick?.Invoke(this);


Shouldn't this be

await State.Options.OnCloseButtonClick.Invoke(this); //with null check

?
since OnCloseButtonClick is Func<Snackbar, Task>. It would probably make sense to add Async suffix.

ScarletKuro · 2024-05-02T23:41:34Z

src/MudBlazor/Components/Snackbar/Snackbar.cs


                // Click action is executed only if it's not from the close icon
-                await State.Options.OnClick.Invoke(this);
+                //await State.Options.OnClick.Invoke(this);
+                State.Options.OnClick.Invoke(this);


Why it's simply not

await State.Options.OnClick.Invoke(this);

?
It's an Func<Snackbar, Task> and if re are renaming, it probably makes sense to name it OnClickAsync
Otherwise I don't get the Clicked change from void to Task
Well, I mean I understand you did it for the ActionClickedAsync / CloseIconClickedAsync, but it makes sense to use await now when it's a Task.

ScarletKuro · 2024-05-02T23:45:26Z

src/MudBlazor/Components/Snackbar/SnackbarOptions.cs

+        /// <summary>
+        /// The asynchronous delegate that is invoked when the close button of the Snackbar is clicked.
+        /// </summary>
+        public Func<Snackbar, Task> OnCloseButtonClick { get; set; }


OnCloseButtonClickAsync?

I disagree, it should be CloseButtonClickFunc.

ScarletKuro · 2024-05-02T23:45:36Z

src/MudBlazor/Components/Snackbar/SnackbarOptions.cs

+        /// <summary>
+        /// The asynchronous delegate that is invoked when the Snackbar is clicked.
+        /// </summary>
+        public Func<Snackbar, Task> OnClick { get; set; }


OnClickAsync?

ScarletKuro · 2024-05-02T23:48:42Z

src/MudBlazor/Components/Snackbar/Snackbar.cs

@@ -30,23 +30,35 @@ internal Snackbar(SnackbarMessage message, SnackbarOptions options)

        internal void Init() => TransitionTo(SnackbarState.Showing);

-        internal void Clicked(bool fromCloseIcon)
+        internal Task Clicked(bool fromCloseIcon)


ClickedAsync.

ScarletKuro · 2024-05-02T23:50:56Z

This is almost done, isn't it?

I don't think so, the build is failing.

Allow to attach custom task on close button click

d8ead67

github-actions bot added enhancement New feature or request PR: needs review labels Apr 6, 2024

BieleckiLtd added 2 commits April 6, 2024 12:00

Added xml docs

e2b6f0b

Make dotnet format happy

bc84c38

danielchalmers reviewed Apr 7, 2024

View reviewed changes

danielchalmers added the breaking change label Apr 7, 2024

Applied review feedbacks

fa53db7

danielchalmers requested review from henon and ScarletKuro April 9, 2024 18:19

danielchalmers reviewed Apr 9, 2024

View reviewed changes

Removed commented out lines of code

516d06f

danielchalmers approved these changes Apr 24, 2024

View reviewed changes

danielchalmers removed the request for review from ScarletKuro April 24, 2024 05:36

henon requested a review from ScarletKuro April 24, 2024 18:09

BieleckiLtd marked this pull request as draft April 27, 2024 12:11

merge dev

e8654e4

ScarletKuro reviewed May 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Snackbar: Allow to attach custom task on close button click #8589

Snackbar: Allow to attach custom task on close button click #8589

BieleckiLtd commented Apr 6, 2024

codecov bot commented Apr 6, 2024 •

edited

danielchalmers left a comment

danielchalmers Apr 7, 2024 •

edited

ScarletKuro May 2, 2024

danielchalmers Apr 9, 2024

BieleckiLtd Apr 9, 2024 •

edited

danielchalmers Apr 24, 2024

henon Apr 24, 2024

henon Apr 24, 2024

Yomodo Apr 25, 2024 •

edited

ScarletKuro May 2, 2024

henon commented May 2, 2024

ScarletKuro May 2, 2024

ScarletKuro May 2, 2024

ScarletKuro May 2, 2024

henon May 3, 2024

ScarletKuro May 2, 2024

ScarletKuro May 2, 2024

ScarletKuro commented May 2, 2024

Snackbar: Allow to attach custom task on close button click #8589

Are you sure you want to change the base?

Snackbar: Allow to attach custom task on close button click #8589

Conversation

BieleckiLtd commented Apr 6, 2024

Description

How Has This Been Tested?

Types of changes

Checklist:

codecov bot commented Apr 6, 2024 • edited

Codecov Report

danielchalmers left a comment

Choose a reason for hiding this comment

danielchalmers Apr 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BieleckiLtd Apr 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yomodo Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henon commented May 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ScarletKuro commented May 2, 2024

codecov bot commented Apr 6, 2024 •

edited

danielchalmers Apr 7, 2024 •

edited

BieleckiLtd Apr 9, 2024 •

edited

Yomodo Apr 25, 2024 •

edited