Snackbar: Allow to attach custom task on close button click#8589
Conversation
github-actions
Bot
added
enhancement
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #8589 +/- ##
==========================================
- Coverage 91.51% 91.50% -0.01%
==========================================
Files 414 414
Lines 12988 12989 +1
Branches 2452 2454 +2
==========================================
Hits 11886 11886
Misses 555 555
- Partials 547 548 +1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
danielchalmers
left a comment
danielchalmers
left a comment
There was a problem hiding this comment.
I like the refactors! Interested in seeing how others feel about CodeInline; Makes sense to me.
| /// <summary> | ||
| /// The asynchronous delegate that is invoked when the Snackbar is clicked. | ||
| /// </summary> | ||
| public Func<Snackbar, Task> OnClick { get; set; } |
There was a problem hiding this comment.
Love the rename. Needs to be added to the Migration Guide if accepted
danielchalmers
added
the
breaking change
| // 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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@henon Thoughts on this behavior? Makes sense to me.
There was a problem hiding this comment.
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");
}
}
There was a problem hiding this comment.
Logger.LogError has an overload that accepts Exception? that might provide stack as well.
Edit: Hm, perhaps not always wanted.
There was a problem hiding this comment.
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.
|
This is almost done, isn't it? |
| if (fromCloseIcon) | ||
| { | ||
| // Invoke user-defined task when close button is clicked | ||
| State.Options.OnCloseButtonClick?.Invoke(this); |
There was a problem hiding this comment.
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.
| // 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); |
There was a problem hiding this comment.
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.
| /// <summary> | ||
| /// The asynchronous delegate that is invoked when the close button of the Snackbar is clicked. | ||
| /// </summary> | ||
| public Func<Snackbar, Task> OnCloseButtonClick { get; set; } |
There was a problem hiding this comment.
I disagree, it should be CloseButtonClickFunc.
There was a problem hiding this comment.
I renamed the property to CloseButtonClickFunc as suggested. Since I'm also correcting Onclick, should it be suffixed with Func too? OnClickFunc, ClickFunc, or just OnClick?
| /// <summary> | ||
| /// The asynchronous delegate that is invoked when the Snackbar is clicked. | ||
| /// </summary> | ||
| public Func<Snackbar, Task> OnClick { get; set; } |
| internal void Init() => TransitionTo(SnackbarState.Showing); | ||
|
|
||
| internal void Clicked(bool fromCloseIcon) | ||
| internal Task Clicked(bool fromCloseIcon) |
I don't think so, the build is failing. |
|
Is there no more interest? We are working on v8, and it's good timing to push any breaking change. |
|
I don't want to push something I don't fully understand. I appreciate the suggested renaming and updating the pull request to align with the current dev state, but I have reservations about two aspects: awaiting and exception handling. My understanding is this:
|
…chronous execution was unnecessary. Added examples for clearer understanding of use cases.
|
I don't remember the code or what it was doing, but if the intention here is not to wait for the user function to finish, because, as you mentioned, "Awaiting longer operations can make the app seem unresponsive" then that’s a valid point. But in that case, you need to indicate this intention, either by using the discard |
|
Will this be added soon to mudblazor I need a OnClose function for the Snackbar |
|
|
I’m happy with this proposal as it is, but I don’t mind if someone else wants to add anything further. |
It will mean that an exception in user code will crash the application, which would be fine. We had instances where exceptions would be swallowed completely because we didn't observe tasks and users complained that it made their code hard to debug. Maybe those discards were done on background threads? I don't know. Anyway, if you are sure that here it will result in a crash then it is ok. |
Yes, the tests to confirm this are in MudBlazor.UnitTests.Viewer |
|
Thanks a lot @BieleckiLtd ! |
|
@BieleckiLtd What should we write in the v8 migration guide? |
|
|
Added to v8.0.0 Migration Guide |
…r#8589) Co-authored-by: Pawel Bielecki <Pawel.Bielecki2@leoni.com>
6 participants
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
Checklist:
dev).