Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allows showing a dialog by only providing a RenderFragment. #1496

Merged
merged 7 commits into from
Feb 13, 2024
Merged

feat: allows showing a dialog by only providing a RenderFragment. #1496

merged 7 commits into from
Feb 13, 2024

Conversation

StevenRasmussen
Copy link
Contributor

Pull Request

📖 Description

Sometimes it is helpful not to have to create a full-blown component to render as a dialog. This PR allows supplying just a RenderFragment to be displayed as a dialog.

👩‍💻 Reviewer Notes

You can see how this works by reviewing the demo site for dialogs. I have added a new section at the bottom that demonstrates the new functionality.

✅ Checklist

General

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

  • I have added a new component - technically, yes, but this is just to help in displaying the dialog component.
  • I have added Unit Tests for my new compontent
  • I have modified an existing component
  • I have validate Unit Tests for an existing component

Sorry, something went wrong.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Copy link
Collaborator

@vnbaaij vnbaaij left a comment

Choose a reason for hiding this comment

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

Also make sure the existing Unit Tests validate and try to add a coule of new ones

@vnbaaij vnbaaij requested a review from dvoituron February 11, 2024 10:06
@dvoituron
Copy link
Collaborator

Could you give us some examples of where this might be an interesting situation?

  1. Personally, I don't think it's a good idea to offer this way of writing Razor code directly in C# code. It's not a good practice, because it's better to distinguish Razor from C#. With the exception of a few cases, such as unit tests or demo code, I think it's dangerous to offer this way of writing by default.

  2. By enabling this method, how can parameters be accessed? If the dev wants to use DialogData, how can he do this?

    public record NameAndAge
    {
        public int Id { get; set; }
        public string? Name { get; set; }
        public int Age { get; set; }
    }
    
    NameAndAge DialogData { get; set; } = new() { Id = 1, Name = "Bill", Age = 42 };
    
    private async Task OpenDialog()
    {
        await _dialogService.ShowDialogAsync(@<div>Content from your render fragment</div>,
                                                new DialogParameters<NameAndAge>
                                                    {
                                                        Title = "Render Fragment Content",
                                                        Content = DialogData,
                                                    });
    }

I'm not against accepting these problems, but to solve what problems?

@StevenRasmussen
Copy link
Contributor Author

StevenRasmussen commented Feb 11, 2024

@dvoituron - I can see why you might be hesitant. I generally agree with your argument here regarding razor markup in C#, and if everyone followed the pattern of always having a separate C# code file for their components then this would hold true. However, there are people that include their C# in the razor file itself, especially for smaller components.

My main reason for this is that I find there are several instances where I want to collect a single input from the user, but find creating an entire component a bit heavy-handed. This is where I see this scenario more compelling. I have used the following in my own code (I currently have this as an extension method of the IDialogService in my solution):

    private async Task RejectCarShowRegistration(CarShowRegistration carShowRegistration)
    {
        var rejectionReason = string.Empty;
        
        // This is where creating an entire component, just to get a single piece of information is a bit of overkill.
        var dialogInstance = await this.DialogService.ShowDialogAsync(
    @<div style="width: 100%;">
        <FluentTextArea @bind-Value=rejectionReason Style="width: 100%" />
    </div>
    , new DialogParameters
    {
        Title = "Rejection Reason",
        PrimaryAction = "Reject",
        PreventDismissOnOverlayClick = true,
    });

        var result = await dialogInstance.Result;
        if (!result.Cancelled)
        {
            await this.PerformOperationSafely(async () =>
            {
                // do something with the 'rejectionReason' here
               
        }
    }

Anyway, I appreciate your consideration in adding this to the project... I feel that others can find this feature useful as well. Let me know if you need anything else to complete this PR.

@dvoituron
Copy link
Collaborator

@StevenRasmussen I understand your request, but it doesn't answer my two questions :-)

  1. Why not create a simple .razor file containing this code? Possibly with a "Dialog type" parameter that would allow multiple windows to be included in a single razor file?

    @if (@Content.MyDialogType  = 1)
    {
        <div style="width: 100%;">
           <FluentTextArea @bind-Value=rejectionReason Style="width: 100%" />
        </div>
    }
  2. By enabling this method, how can parameters be accessed?

The biggest problem for me is that we're going to present a way of developing that doesn't seem ideal to me, when it would be possible to achieve the same result without too much extra work. Am I wrong?

@StevenRasmussen
Copy link
Contributor Author

@dvoituron - To answer your questions:

Why not create a simple .razor file containing this code? Possibly with a "Dialog type" parameter that would allow multiple windows to be included in a single razor file?

Certainly this could be a solution. I'm not contending that everything should be done using this new approach. I guess it comes down to a matter of opinion (which is often the case with different approaches to coding 😉). For me personally, I would not want to create a component that serves many different purposes, just for the sake of having a component that could be used for the FluentDialog component. This would seem to violate the single responsibility principle in my opinion.

By enabling this method, how can parameters be accessed?

In the instance where a RenderFragment is being used as the body of the dialog, you would not pass any DialogData as you did in your example above. The render fragment would simply inline any data that it needs into the fragment itself. You can still bind values to inputs if you want, as I demonstrated in the example I posted above. The rejectionReason is a string value that gets bound to a FluentTextArea control. Once the dialog result is evaluated to ensure that it wasn't cancelled, the rejectionReason is now populated and I can do what I need with it. The nice thing about this whole concept is that the logic is all isolated to a single function. You don't have to go search a different file or component for something so simple and the data can be bound to a local variable (which I suppose you can do with a regular component as well).

In the end it's going to come down to your opinion of whether you find this could be useful for other developers or not. I don't think that one approach is "correct" and one is "wrong" (I use both in my codebase). I think it's a matter of choosing the right tool and approach for each scenario. This feature naturally targets very simple dialogs, ones that collect 1 or 2 pieces of data from a user or perhaps just displays some simple text in a custom format. In my opinion it is perfectly fine (and even preferred) to inline the dialog code right into the method that is invoking it instead of creating another file for something that is so simple and will only ever be used in this very specific instance.

Anyway, I really appreciate all the effort that your team has put into this library... just thought that I'd add something to it that I find really useful and that I think other developers will appreciate too.

@dvoituron
Copy link
Collaborator

Indeed, we could add this way of displaying DialogBoxes, perhaps some developers like to work this way (especially since Blazor allows it) :-)

I'll just add a comment for this new method, pointing out that this parameter is not possible / useful.
new DialogParameters<TContent>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@vnbaaij vnbaaij enabled auto-merge (squash) February 13, 2024 08:33
@vnbaaij vnbaaij merged commit 144c17e into microsoft:dev Feb 13, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants