chanan / BlazorStrap

Bootstrap 4 Components for Blazor Framework
https://blazorstrap.io
The Unlicense
917 stars 157 forks source link

BlazorInput two-way binding doesn't work #41

Closed dinhduongha closed 5 years ago

dinhduongha commented 5 years ago

When using the BlazorInput tag you are unable to use two-way binding to the value. Change text and press submit.

<BlazorInput InputType="InputType.Text" bind-Content="@Content" />@*Two-Way?*@
<input type="text" bind="@Content2" />
<BlazorButton Color="Color.Primary" OnClick="@Submit">Submit</BlazorButton>
@functions {
    private string Content { get; set; }
    private string Content2 { get; set; }
    async void Submit(UIMouseEventArgs e)
    {
        Console.WriteLine(Content);  // Not work
        Console.WriteLine(Content2); // Work
    }
}
IvanJosipovic commented 5 years ago

Change bind-Content to bind-Value.

Have a look at, https://github.com/chanan/BlazorStrap/blob/master/src/BlazorStrap/BlazorInput.cshtml [Parameter] private string Value { get; set; }

simbesh commented 5 years ago

@IvanJosipovic I cannot get binding to work also. is onchange=@onchange missing from the attributes list to call the method? When I created my own component I added that and it worked.

razfriman commented 5 years ago

Can confirm this is not functioning properly. Here is my usage.

Tried various combinations of bind, value, etc.

One way binding works, but unfortunately until I can get two way data binding working I cannot start to use this library in my project :(

<FormGroup>
<BlazorLabel For="inputFirstName">First Name</BlazorLabel>
<BlazorInput Id="inputFirstName" InputType="InputType.Text" PlaceHolder="First Name" bind-Value="@user.FirstName" />
</FormGroup>
<FormGroup>
<BlazorLabel For="inputFirstName">First Name</BlazorLabel>
<BlazorInput Id="inputFirstName" InputType="InputType.Text" PlaceHolder="First Name" Value="@user.FirstName" />
</FormGroup>
chanan commented 5 years ago

bind-value should work, I will take a look at this when I look at the other issue with form tonight.

chanan commented 5 years ago

The code looks right to me based on the docs: https://docs.microsoft.com/en-us/aspnet/core/blazor/components?view=aspnetcore-3.0#data-binding Not sure what is wrong.

chanan commented 5 years ago

Tried adding:

case EventCallback<UIChangeEventArgs> e:
                        builder.AddAttribute(1, param.Key, e);
                        break;

Didnt help either

chanan commented 5 years ago

I tracked down the problem, but not sure how to solve yet. The issue is that DynamicElement isn't updating value, it most likely need to build an on change event.

razfriman commented 5 years ago

Nice! I'll keep looking into it when I have some time.. So far I have reproduced it in my branch to the point where BlazorInput acts as 1-way, and normal input successfully works as two-way: feel free to check out my branch where I was exploring it:

https://github.com/razfriman/BlazorStrap/pull/1

chanan commented 5 years ago

I got basic two way binding support working:

  1. Works on strings in BlazorInput, not sure checkboxs will work or select
  2. Seems to update when the form is submitted only

Creating a new build now, I also add /samples/form with a working example.

razfriman commented 5 years ago

Have you had a look at this? This is the implmentation for Blazor's EditForm Input element variants? Might be a good example to follow?

https://github.com/aspnet/AspNetCore/blob/master/src/Components/Components/src/Forms/InputComponents/InputText.cs

chanan commented 5 years ago

I am considering changing either just the form/input control or all controls to be actual C# classes instead of razor files (which have a backing C# class anyway). At the same time also take some cues from EditForm control. Any thoughts about this?

chucker commented 5 years ago

Any thoughts about this?

I mean… the linked page does say "Developers building their own input components should use Razor syntax." ;-)

chucker commented 5 years ago

I am considering changing either just the form/input control or all controls to be actual C# classes instead of razor files (which have a backing C# class anyway).

Can we separate that discussion into its own issue?

Creating a new build now, I also add /samples/form with a working example.

Not sure if I broke that, but this fails on load with:

blazor.webassembly.js:1 WASM:
System.ArgumentException: Object of type
'Microsoft.AspNetCore.Components.EventCallback`1[Microsoft.AspNetCore.Components.UIEventArgs]'
cannot be converted to type 'Microsoft.AspNetCore.Components.EventCallback'.
chanan commented 5 years ago

About the binding - I wont want to do anything till 1.0 is final - lets talk about it again at that time?

About the error - I will take a look tonight, but I was also wondering if this is still needed: https://github.com/chanan/BlazorStrap/blob/master/src/BlazorStrap/DynamicElement.cs#L90-L131

chucker commented 5 years ago

I was also wondering if this is still needed:

Oh, interesting. They closed https://github.com/aspnet/AspNetCore/pull/10357.

On the one hand, that's great — I considered that piece of code a hack. :)

On the other hand, I fear we found the reason for my error above.

rpskelton2 commented 5 years ago

From looking at documentation and samples it appears the razor.g.cs file should contain onchange statements like below. I'm not any in mine and two way binding is not working on any of my samples.

builder.AddAttribute(5, "onchange", Microsoft.AspNetCore.Components.EventCallback.Factory.CreateBinder(this, value => Title = value, Title));

chanan commented 5 years ago

Having some issues upgrading to preview6, I will take another look at this once that is done

rpskelton2 commented 5 years ago

It seems the syntax for binding has changed. Most examples found on line look like this and do not work: input type="text" bind="@Name" />

The new syntax with @bind seems ok: input type="text" @bind="Name" />

This commit on May 30 changed the syntax: https://github.com/aspnet/AspNetCore/commit/dd07fa09d2895bba3231d9e433da1a4671630512#diff-438bb5dff82affac680fa750d6535658

chucker commented 5 years ago

Yes, that's correct. In preview 6, you're actually supposed to use @bind="@Name". They're hoping to eventually shorten that to @bind="Name". But bind without the @ is no longer correct — attributes without the @ are now HTML, and attributes with it C#.

chanan commented 5 years ago

I updated the syntax in v1.0.0-preview6-10 please let me know what you think

chanan commented 5 years ago

Current status:

Per current documentation (preview6) BlazorIntput has:

[Parameter] private string Value { get; set; }
[Parameter] private EventCallback<string> ValueChanged { get; set; }

Which seems to work more or less ok (I think there are still some issues). I am hesitant to change more for two reasons:

  1. more changes to Blazor might change how binding works. Specifically for BlazorStrap, Blazor will be adding splat operator which will likely mean we won't need BootstrapComponentBase anymore which may effect some of our binding issues
  2. I think we should either base forms/inputs off of EditForm or supply duplicates that use EditForm which will also likely solve some of our binding issues

Due to the above, I am going to wait a bit and see what Blazor brings before attempting to further solve these issues.

PS - Basic binding does work, please try it out and let me know how it goes