MudDialogProvider: Add CloseOnNavigation to DialogOptions to optionally close dialogs on navigation#12437
Conversation
… closed on navigation.
Note: The DismissAll function is copied over instead of edited, as it is public, to avoid unnecessary api changes.
Add test dialogs being dismissed when navigating when the CloseOnNavigation option is false.
…th a real NavigationManager we need to use BunitNavigationManager instead of MockNavigationManager.
… null instead of default to true.
…default value (null)
|
I have add a solution according to your comment. We still want to keep CloseOnNavigation as an option as this doesn't address pages that have multiple |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -67,6 +72,12 @@ public interface IDialogReference | |||
| /// <param name="inst">The new dialog to use.</param> | |||
| void InjectDialog(object inst); | |||
|
|
|||
| /// <summary> | |||
| /// Replaces the dialog options. | |||
| /// </summary> | |||
| /// <param name="options">The new options to use</param> | |||
| void InjectOptions(DialogOptions options); | |||
|
|
|||
There was a problem hiding this comment.
IDialogReference is a public interface; adding new members (Options, InjectOptions) is a binary breaking change for any external implementation of IDialogReference. If maintaining backwards compatibility is a goal, consider introducing a new derived interface (e.g., IDialogReferenceWithOptions) or providing default interface implementations where possible.
There was a problem hiding this comment.
I believe this is an acceptable breaking change, otherwise where does it end.
| var reference = await service.ShowAsync<DialogOkCancel>(); | ||
| reference.InjectOptions(null); | ||
|
|
There was a problem hiding this comment.
InjectOptions(null) does not compile with the current IDialogReference.InjectOptions(DialogOptions options) signature. Either make InjectOptions accept nullable options, or update these tests to avoid passing null (e.g., verify behavior with CloseOnNavigation = null on a real DialogOptions instance).
There was a problem hiding this comment.
This is a test, options are not required on construction of a reference, neither is the RenderFragment or the Dialog instance itself. This is edge case test for a poorly configured IDialogReference.
It will compile it just has a warning on the test case.
(e.g., verify behavior with CloseOnNavigation = null on a real DialogOptions instance). This is verified in other tests in the same file.
| var reference = await service.ShowAsync<DialogOkCancel>(); | ||
| reference.InjectOptions(null); | ||
|
|
There was a problem hiding this comment.
InjectOptions(null) does not compile with the current IDialogReference.InjectOptions(DialogOptions options) signature. Either make InjectOptions accept nullable options, or update these tests to avoid passing null (e.g., verify behavior with CloseOnNavigation = null on a real DialogOptions instance).
There was a problem hiding this comment.
This is a test, options are not required on construction of a reference, neither is the RenderFragment or the Dialog instance itself. This is edge case test for a poorly configured IDialogReference.
It will compile it just has a warning on the test case.
(e.g., verify behavior with CloseOnNavigation = null on a real DialogOptions instance). This is verified in other tests in the same file.
| /// Replaces the dialog options. | ||
| /// </summary> | ||
| /// <param name="options">The new options to use</param> | ||
| void InjectOptions(DialogOptions options); |
There was a problem hiding this comment.
Nullability is inconsistent: Options is declared as DialogOptions?, but InjectOptions requires a non-null DialogOptions. Either make Options non-nullable (and ensure it's always set at creation) or allow InjectOptions(DialogOptions? options) so callers/tests can represent "no options" consistently.
| void InjectOptions(DialogOptions options); | |
| void InjectOptions(DialogOptions? options); |
There was a problem hiding this comment.
This is consistent with similar functionality in IDialogReference. The options are nullable as they are optional at creation, that doesn't necessarily mean you can later set them to null.
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public void InjectOptions(DialogOptions options) |
There was a problem hiding this comment.
DialogReference.InjectOptions cannot set Options back to null because the parameter is non-nullable. This currently conflicts with new unit tests that call InjectOptions(null). Align the signature with the nullable Options property (accept DialogOptions?) or remove the need to ever represent Options == null and adjust tests accordingly.
| public void InjectOptions(DialogOptions options) | |
| public void InjectOptions(DialogOptions? options) |
There was a problem hiding this comment.
This is in reference to a unit test, it is an object reference so unlike a struct nullability is a suggestion. The call to .InjectOptions(null) is for code converge reasons as DialogOptions can be null on the DialogReference.
| var newUri = $"{uri.GetLeftPart(UriPartial.Path)}?{query}"; | ||
| await comp.InvokeAsync(() => navigationManager.NavigateTo(newUri)); | ||
|
|
||
| cont = comp.FindAll("div.mud-dialog-container"); |
There was a problem hiding this comment.
This test expects only 1 dialog to remain after changing only the query string, but the new CloseOnNavigation == null behavior is documented to not close dialogs when only query/fragment changes. After fixing _currentUri initialization, this test will likely fail; update it to assert the intended default behavior (query change keeps dialogs open, path change closes unless CloseOnNavigation == false).
| var newUri = $"{uri.GetLeftPart(UriPartial.Path)}?{query}"; | |
| await comp.InvokeAsync(() => navigationManager.NavigateTo(newUri)); | |
| cont = comp.FindAll("div.mud-dialog-container"); | |
| var newUriWithQueryChange = $"{uri.GetLeftPart(UriPartial.Path)}?{query}"; | |
| await comp.InvokeAsync(() => navigationManager.NavigateTo(newUriWithQueryChange)); | |
| // Changing only the query string should not close dialogs when CloseOnNavigation is null (default) | |
| cont = comp.FindAll("div.mud-dialog-container"); | |
| cont.Count.Should().Be(2); | |
| // Changing the path should close dialogs with default CloseOnNavigation, but keep those with CloseOnNavigation = false | |
| var newUriWithPathChange = $"{uri.GetLeftPart(UriPartial.Path)}-other"; | |
| await comp.InvokeAsync(() => navigationManager.NavigateTo(newUriWithPathChange)); | |
| cont = comp.FindAll("div.mud-dialog-container"); |
| @@ -91,6 +92,39 @@ public async Task Simple() | |||
| result.Canceled.Should().BeFalse(); | |||
| } | |||
|
|
|||
| /// <summary> | |||
| /// Opening and closing dialogs via navigation. | |||
| /// </summary> | |||
| [Test] | |||
| public async Task CloseOnNavigationTest() | |||
| { | |||
| var comp = Context.Render<MudDialogProvider>(); | |||
| comp.Markup.Trim().Should().BeEmpty(); | |||
| var dialogService = Context.Services.GetRequiredService<IDialogService>(); | |||
| dialogService.Should().NotBe(null); | |||
| var navigationManager = Context.Services.GetRequiredService<NavigationManager>(); | |||
| navigationManager.Should().NotBe(null); | |||
|
|
|||
| //create 2 instances and dismiss all except for one with CloseOnNavigation = false | |||
| var closeOnNavigationOptions = new DialogOptions | |||
| { | |||
| CloseOnNavigation = false | |||
| }; | |||
| await comp.InvokeAsync(async () => _ = await dialogService.ShowAsync<DialogOkCancel>("test", options: closeOnNavigationOptions)); | |||
| await comp.InvokeAsync(async () => _ = await dialogService.ShowAsync<DialogOkCancel>()); | |||
| var cont = comp.FindAll("div.mud-dialog-container"); | |||
| cont.Count.Should().Be(2); | |||
|
|
|||
| var uri = new Uri(navigationManager.Uri); | |||
| var query = HttpUtility.ParseQueryString(uri.Query); | |||
| query["query"] = Guid.NewGuid().ToString(); | |||
| var newUri = $"{uri.GetLeftPart(UriPartial.Path)}?{query}"; | |||
| await comp.InvokeAsync(() => navigationManager.NavigateTo(newUri)); | |||
There was a problem hiding this comment.
Introducing using System.Web; / HttpUtility.ParseQueryString adds a new dependency that does not appear to be referenced in the test project and may fail to compile on modern .NET targets. Prefer a framework-native approach (e.g., UriBuilder, simple string concat, or Microsoft.AspNetCore.WebUtilities.QueryHelpers) to construct a URI with updated query parameters.
| @@ -7,6 +10,8 @@ | |||
| <MudButton OnClick="ToggleCloseButtonAsync">Toggle Close Button</MudButton> | |||
| <MudButton OnClick="ToggleFullWidthAsync">Toggle Full Width</MudButton> | |||
| <MudButton OnClick="ToggleHeaderAsync">Toggle Header</MudButton> | |||
| <MudCheckBox TriState="true" T="bool?" Value="MudDialog.Options.CloseOnNavigation" ValueChanged="@(async v => await CloseOnNavigationChangedAsync(v))">Close On Navigation</MudCheckBox> | |||
| <MudButton OnClick="ChangeUrl">Change Url</MudButton> | |||
| </div> | |||
| </DialogContent> | |||
| <DialogActions> | |||
| @@ -18,6 +23,13 @@ | |||
| [CascadingParameter] | |||
| private IMudDialogInstance MudDialog { get; set; } | |||
|
|
|||
| [SupplyParameterFromQuery(Name = "example")] | |||
| public int? QueryParameterExample { get; set; } | |||
|
|
|||
| protected override void OnParametersSet() { | |||
| QueryParameterExample ??= 0; | |||
| } | |||
|
|
|||
| private void Close() => MudDialog.Close(DialogResult.Ok(true)); | |||
|
|
|||
| private Task ChangeTitleAsync() => MudDialog.SetTitleAsync($"Current time is: {DateTime.Now}"); | |||
| @@ -51,4 +63,26 @@ | |||
|
|
|||
| return MudDialog.SetOptionsAsync(options); | |||
| } | |||
|
|
|||
| private void ChangeUrl() { | |||
| var uri = new Uri(NavigationManager.Uri); | |||
|
|
|||
| var query = HttpUtility.ParseQueryString(uri.Query); | |||
|
|
|||
| query["example"] = (QueryParameterExample + 1).ToString(); | |||
|
|
|||
| var newUri = $"{uri.GetLeftPart(UriPartial.Path)}?{query}"; | |||
|
|
|||
| NavigationManager.NavigateTo(newUri); | |||
There was a problem hiding this comment.
@using System.Web / HttpUtility.ParseQueryString introduces a dependency that isn't referenced in the docs project (and is generally avoided in Blazor). Since MudBlazor.Docs already references Microsoft.AspNetCore.WebUtilities, consider using QueryHelpers.AddQueryString (or UriBuilder) to update the query string without relying on System.Web APIs.
|
Sorry about Copilot - Thank you for all your work on this!! |
…ly close dialogs on navigation (MudBlazor#12437) Co-authored-by: avalerio <avalerio@tnwops.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Daniel Chalmers <daniel.chalmers@outlook.com>
Added
CloseOnNavigationproperty toDialogOptionsand implemented the use case for that property.This use case also required that the
IDialogReferencenow have access to theDialogOptionsthat were supplied during its creation.This change fixes an issue with same page navigation where url changes automatically close all dialogs regardless of the navigation destination being somewhere different or the same page simply updating a parameter.
Checklist: