microsoft / fluentui-blazor

Microsoft Fluent UI Blazor components library. For use with ASP.NET Core Blazor applications
https://www.fluentui-blazor.net
MIT License
3.79k stars 366 forks source link

feat: add proper generic binding to components #2471

Open MarvinKlein1508 opened 2 months ago

MarvinKlein1508 commented 2 months ago

🙋 Feature Request

This feature request is connected to my discussion, which can be found here: https://github.com/microsoft/fluentui-blazor/discussions/2443

This issue should collect and exchange feedback to implement proper generic binding in most components.

Many components rely on some kind of underlying datatype. This results in many code for converting stuff just to use certain components.

FluentSelect

Binding to this component always require either an object or a string to be bound. This component should allow binding to any kind of value.

Example for vanilla Blazor:

<InputSelect @bind-Value="Input.MyNumber">
    <option value="">--- Select ---</option><!-- This handles null -->
    <option value="0">0</option>
    <option value="1">1</option>
</InputSelect>

@code {
    public MyInput Input { get; set; } = new();

    public class MyInput 
    {
        public int? MyNumber { get; set; }
    }
}

Or for enums:

<InputSelect @bind-Value="Input.Day">
    foreach (var item in Enum.GetValues<DayOfWeek>())
    {
        <option value="@item">@item</option>
    }
</InputSelect>

@code {
    public MyInput Input { get; set; } = new();

    public class MyInput 
    {
        public DayOfWeek Day { get; set; } = DayOfWeek.Monday;
    }
}

Perhaps this component can be splitted into two parts, one for basic binding (int, string, Guid, enum, int?, string?, Guid?, enum?) and the other to properly bind to complex objects only.

In vanilla Blazor binding objects can be achieved like this:

<InputSelect TValue="DayOfWeek" 
             Value="Input.Day"
             ValueChanged="OnValueChanged"
             ValueExpression="() => Input.Day">
    foreach (var item in Enum.GetValues<DayOfWeek>())
    {
        <option value="@item">@item</option>
    }
</InputSelect>

@code {
    public MyInput Input { get; set; } = new();

    private void OnValueChanged(DayOfWeek newValue)
    {
        Input.Day = newValue;

        // This is where you can set objects for the input based on the value
        Input.OtherProperty = "Set other properties from the Input";
        // You'll need to notify EditCOntext for the change of the other properties yourself though.
    }

    public class MyInput 
    {
        public DayOfWeek Day { get; set; } = DayOfWeek.Monday;

        public string? OtherProperty { get; set; }
    }
}

FluentDatePicker

This component only allows to bind DateTime? values. It should support other date formats out of the box.

FluentAutoComplete

We basically need this component as FluentSelect component to allow easy bidning on just 1 item. Please support generics here as well. You can only bind IEnumerable<T> here and not List<T>

JarJasAskom commented 2 months ago

Would it be possible to clarify a bit "vNext" label? Could the next major version be expected this year?

I have a model that contains a field of enum type. This field is validated elsewhere and the result is added to ValidationMessageStore/EditContext.

Without FluentSelect supporting binding to the field of enum type it seems impossible to use it in this scenario - support for showing validation errors and support for OnFieldChange event in EditContext.

vnbaaij commented 2 months ago

We never give dates on when next version will be released.

We have a lot of changes, enhancements, etc planned, so no idea if it will be done this year. Also depends on when we components v3 will be ready AND complete enough.

MarvinKlein1508 commented 2 months ago

@JarJasAskom you can bind to an enum with FluentSelect but it requires a lot of manually work right now.

<FluentSelect TOption="MyEnum" Label="Select object" Width="100%;">
     <FluentOption Value="null" Selected="@(Input.MyEnum is null)" OnSelect="(val) => Input.MyEnum = null">--- Select ---</FluentOption>
     @foreach (var item in Enum.GetValues<MyEnum>())
     {
         <FluentOption TOption="MyEnum" Value="@item.ToString()" Selected="@(Input.MyEnum == item)" OnSelect="(val) => Input.MyEnum = item">@item</FluentOption>
     }
 </FluentSelect>
adamgf1 commented 2 weeks ago

@MarvinKlein1508, for this discussion I would just caution (in case you've not already discovered this) that your examples are overwriting parameters, which is strongly discouraged in the documentation.

That's this kind of thing in your example:

    private void OnValueChanged(DayOfWeek newValue)
    {
        Input.Day = newValue;

        // This is where you can set objects for the input based on the value
        Input.OtherProperty = "Set other properties from the Input";
        // You'll need to notify EditCOntext for the change of the other properties yourself though.
    }

That snippet is also an example of how two-way data binding shouldn't be done, as per this page in the documentation.

I'm not saying that to throw you under the bus, but because I actually got to this issue while I was trying to find out if there was any discussion here about implementing first-class support for one-way binding with the Fluent UI Blazor components. (It's also possible I'm just misunderstanding things about your specific use-case, as I haven't really delved into forms and EditorContext so far, in which case apologies.)

Regarding one-way binding again though, at least part of the struggle we're having in our team seems to be down to the Fluent UI Blazor components internally overwriting their parameters, like I'm understanding your example to be suggesting, so I wanted to jump in and raise that point here @vnbaaij, in case the intention is continue doing that again with the next major version of the Blazor components? (Which I understand is a big change, and potentially a ground-up rewrite to use the v3 web components, and so taking it as an opportunity to no longer overwrite parameters in the Blazor implementation may be on the cards?)

Basically, what we're finding is that our ability to reliably one-way bind to the Fluent UI Blazor components is very limited, with this at least partially being down to the parameter overwriting occurring in the Blazor component implementations (and, to be fair, it does also seem that there's a bit of a fundamental limitation within the current major version of the underlying web components in this respect too, so I appreciate it's not entirely in control of the Blazor package to fix).

And if it's not clear, when I say one-way binding, I'm talking about the Fluent UI Blazor components being able to support the use-case of their state being dictated entirely by their parameter values, with their EventCallbacks giving you the opportunity to alter or prevent a change to their state entirely using the @bind:get/@bind:set modifiers (as per dotnet/aspnetcore#39837).

I realise that's all become a bit of a derailment from the topic of the original issue, so I can raise a separate issue for this request/question if you think it's worth further discussion @vnbaaij (and it's not something already on your radar for the upcoming major version of the Blazor components). Or, if one-way binding is just not something that's intended to be supported with the Blazor Fluent UI package, then that's fine and I can end my side-track of the discussion here.

vnbaaij commented 2 weeks ago

And if it's not clear, when I say one-way binding, I'm talking about the Fluent UI Blazor components being able to support the use-case of their state being dictated entirely by their parameter values, with their EventCallbacks giving you the opportunity to alter or prevent a change to their state entirely using the @bind:get/@bind:set modifiers (as per dotnet/aspnetcore#39837).

I don't think this is called (or gives you) one way binding but that is an interpretation detail. We have not designed the components to work with these @bind:get/set modifiers simple because the did notexist when most of the componnets were initially built. It can be something we look at for v5 (but no guarantees). Unless you think something is missing from what already described here, this issue is labeled vNext already so we will take it into account

adamgf1 commented 2 weeks ago

No that's plenty good enough, thanks @vnbaaij. I appreciate there's a lot of work involved in maintaining a library like this, especially as new features come into Blazor itself like you say.

And yes, you are definitely right that what I started describing with @bind:get/@bind:set is also two-way binding, not one-way binding. I guess what I'm really trying to describe there is using the more powerful two-way binding features in Blazor to try to get components to behave more like they're doing one-way binding, when they may otherwise be built and intended for two-way binding.

When I put it that way, it starts to sound like a bit of a dangerous workaround, but then again I think the example of using preventDefault with a standard input element is a classic example of the same kind of approach that gets widely used to achieve one-way binding (just that it's with standard DOM elements and events rather than Blazor components). So I believe (but could definitely be wrong) that using @bind:set is about as close to preventDefault as you can get when it comes to Blazor components, to get a component that otherwise normally mutates its own state to instead leave its own state alone and allow you to fully drive it from your own data/state (which is my understanding of what makes for one-way binding).

But yes, I still realise that's really just an "interpretation" of one-way binding, and that using @bind:set to do a no-op (or to immediately revert a component's parameter value to its previous value before the next render, in the case where the component has already mutated its parameter values internally), is still not the same thing as preventing the component from changing its internal state in the first place, as is happening when e.g. @onkeydown:preventDefault gets used.

Anyway, that's another side track from me. Circling back around to the beginning, I think having the Fluent UI Blazor components not overwrite their own parameters would go a long way to supporting one-way binding and/or @bind:get/@bind:set, but I'm sure there's a lot more work involved in it than that, and you obviously have a much more in-depth understanding of that than I do. So as I say, just an acknowledgement of the request and the aspiration to support these things (with no guarantees) in v5 is as much as I can possibly ask for. So thanks for that.

I'll leave it there. Thanks for all the effort and support of this library so far.