Skip to content

Binding support for 'bool' values with InputRadioGroup and InputSelect #35318

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

Merged
merged 5 commits into from
Aug 19, 2021

Conversation

MackinnonBuck
Copy link
Member

Binding support for bool values with InputRadioGroup and InputSelect

This PR adds support for bool value bindings with the InputRadioGroup and InputSelect components.

PR Description

Added special-cased bool value conversion logic shared between the InputRadioGroup and InputSelect components to address #34816.

Why this special-cased approach?
A general-purpose approach to binding bool values to attributes might seem appealing at first, but I've determined that this may be infeasible without a huge amount of refactoring. The framework currently assumes that binding a bool value to an attribute means the attribute is conditional (i.e. a boolean attribute - it exists or it doesn't). Among other things, we would have to introduce new mechanisms to decide whether a given attribute is conditional or string-valued in order to implement a generalized solution.

Changing the existing bool converter to also convert string values provides one-way binding (JS->.NET), but as soon as we attempt a .NET->JS update, the bound attribute gets bool-ified again. Changing the formatter to always format to a string won't fix this because the framework relies on the distinction between bool and string values after they've gone through the formatter to determine if the attribute is conditional.

Other thoughts
If we determine that it's too weird to only have this special bool binding behavior for InputRadioGroup and InputSelect, it's probably fine that we don't accept this PR. IMO, it's a bit strange to be using select and radio inputs rather than checkboxes for bool values. But, if the developer really wants to, they could already make it work by binding to a string instead and performing their own conversion, or (more reasonably) by using an enum that accurately represents the choice being made.

Fixes #34816

@MackinnonBuck MackinnonBuck requested a review from a team as a code owner August 13, 2021 00:27
@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Aug 13, 2021
@MackinnonBuck MackinnonBuck requested a review from pranavkm August 13, 2021 00:28
Copy link
Contributor

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

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

Personally think this is a reasonable change to take. I'll defer to @SteveSandersonMS since he's the one most familiar with BindConverter (outside of you 👍🏽 )

T/F: 7 * 3 = 21<br>
<InputRadioGroup @bind-Value="person.IsRadioMathStatementTrue">
<InputRadio Value="true" />true<br>
<InputRadio Value="false" />false<br>
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing that if the code here was Value="@true" and Value="@false" it would still work the same, would it?

If I'm reading the code here correctly, <InputRadio Value="true" /> will be an InputRadio<string> rather than an InputRadio<bool>. Does it make any difference if the developer makes it into an InputRadio<bool>?

I think it doesn't make any difference because the binding machinery is really on the InputRadioGroup which will be an InputRadioGroup<bool> in both cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since Value is a parameter of InputRadio, not an HTML attribute, isn't 'true' interpreted as the value true rather than the string "true"? In other words, I think true and @true are equivalent here (correct me if I misinterpreted your comment).

That said, the <InputSelect> case is a little weird in exactly this regard:

<InputSelect @bind-Value="ignoreTheTypeOfThis">
    <!-- We can set the value attribute to an enum, because the bind formatter properly serializes it -->
    <option value="@SomeEnum.Choice">Choice</option> <!-- Good -->

    <!-- But we can't do this with a bool, because now 'value' is treated as a conditional attribute -->
    <option value="@true">true</option> <!-- Bad! -->

    <!-- So, we have to use a string -->
    <option value="true">true</option> <!-- Good? -->
</InputSelect>

Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

This looks ideal to me!

I agree with your analysis about the value of implementing this in InputSelect and InputRadio specifically rather than changing the much broader landscape of rendering and binding logic to make it work on raw elements. In other words, I agree it seems valuable enough to implement for those components, but not universal enough to warrant changing everything for it.

Thanks!

@MackinnonBuck MackinnonBuck merged commit 8f871c4 into main Aug 19, 2021
@MackinnonBuck MackinnonBuck deleted the t-mbuck/input-boolean-value-bindings branch August 19, 2021 17:52
@ghost ghost added this to the 7.0-preview1 milestone Aug 19, 2021
wtgodbe pushed a commit that referenced this pull request Aug 20, 2021
…oup and InputSelect (#35523)

* Binding support for 'bool' values with InputRadioGroup and InputSelect (#35318)

* Update CodeCheck.ps1

Co-authored-by: Brennan <[email protected]>
dougbu added a commit that referenced this pull request Aug 24, 2021
* [release/6.0-rc1] Update dependencies from dotnet/runtime dotnet/efcore (#35513)

* Set HttpSys read error log levels to debug #35490 (#35542)

Co-authored-by: Chris R <[email protected]>

* [release/6.0-rc1] Treat reference type parameters in oblivious nullability context as optional (#35526)

* Treat parameters in oblivious nullability context as optional

* Only apply fix for reference types

* Update optionality check in API descriptor

* Update check in BindAsync and Mvc.ApiExplorer test

* HTTP/3: Use new QuicStream.ReadsCompleted property in transport (#35483)

Co-authored-by: James Newton-King <[email protected]>

* HTTP/3: Fix incorrectly pooling aborted streams (#35441)

* [release/6.0-rc1] Binding support for 'bool' values with InputRadioGroup and InputSelect (#35523)

* Binding support for 'bool' values with InputRadioGroup and InputSelect (#35318)

* Update CodeCheck.ps1

Co-authored-by: Brennan <[email protected]>

* [release/6.0-rc1] Update dependencies from dotnet/efcore dotnet/runtime (#35558)

* Update dependencies from https://github.com/dotnet/efcore build 20210820.19

Microsoft.EntityFrameworkCore.Tools , dotnet-ef , Microsoft.EntityFrameworkCore , Microsoft.EntityFrameworkCore.SqlServer , Microsoft.EntityFrameworkCore.InMemory , Microsoft.EntityFrameworkCore.Relational , Microsoft.EntityFrameworkCore.Sqlite , Microsoft.EntityFrameworkCore.Design
 From Version 6.0.0-rc.1.21420.8 -> To Version 6.0.0-rc.1.21420.19

* Update dependencies from https://github.com/dotnet/efcore build 20210820.22

Microsoft.EntityFrameworkCore.Tools , dotnet-ef , Microsoft.EntityFrameworkCore , Microsoft.EntityFrameworkCore.SqlServer , Microsoft.EntityFrameworkCore.InMemory , Microsoft.EntityFrameworkCore.Relational , Microsoft.EntityFrameworkCore.Sqlite , Microsoft.EntityFrameworkCore.Design
 From Version 6.0.0-rc.1.21420.8 -> To Version 6.0.0-rc.1.21420.22

* Update dependencies from https://github.com/dotnet/efcore build 20210820.30

Microsoft.EntityFrameworkCore.Tools , dotnet-ef , Microsoft.EntityFrameworkCore , Microsoft.EntityFrameworkCore.SqlServer , Microsoft.EntityFrameworkCore.InMemory , Microsoft.EntityFrameworkCore.Relational , Microsoft.EntityFrameworkCore.Sqlite , Microsoft.EntityFrameworkCore.Design
 From Version 6.0.0-rc.1.21420.8 -> To Version 6.0.0-rc.1.21420.30

* Update dependencies from https://github.com/dotnet/runtime build 20210820.15

Microsoft.NETCore.Platforms , Microsoft.NETCore.BrowserDebugHost.Transport , Microsoft.NETCore.App.Runtime.win-x64 , Microsoft.Win32.SystemEvents , Microsoft.NET.Runtime.MonoAOTCompiler.Task , Microsoft.Internal.Runtime.AspNetCore.Transport , Microsoft.Extensions.Primitives , Microsoft.Extensions.Options.DataAnnotations , Microsoft.Extensions.Options.ConfigurationExtensions , Microsoft.NET.Runtime.WebAssembly.Sdk , Microsoft.Extensions.Options , Microsoft.Extensions.Logging.TraceSource , Microsoft.Extensions.Logging.EventSource , Microsoft.Extensions.Configuration.UserSecrets , Microsoft.Extensions.Configuration.Json , Microsoft.Extensions.Configuration.Ini , Microsoft.Extensions.Configuration.FileExtensions , Microsoft.Extensions.Configuration.EnvironmentVariables , Microsoft.Extensions.Configuration.CommandLine , Microsoft.Extensions.Configuration.Binder , Microsoft.Extensions.Configuration.Xml , Microsoft.Extensions.Configuration.Abstractions , Microsoft.Extensions.Caching.Memory , Microsoft.Extensions.Caching.Abstractions , Microsoft.Extensions.Configuration , Microsoft.Extensions.DependencyInjection , Microsoft.Extensions.DependencyInjection.Abstractions , Microsoft.Extensions.Logging.EventLog , Microsoft.Extensions.Logging.Debug , Microsoft.Extensions.Logging.Console , Microsoft.Extensions.Logging.Configuration , Microsoft.Extensions.Logging.Abstractions , Microsoft.Extensions.Logging , Microsoft.Extensions.Http , Microsoft.Extensions.Hosting.Abstractions , Microsoft.Extensions.Hosting , Microsoft.Extensions.HostFactoryResolver.Sources , Microsoft.Extensions.FileSystemGlobbing , Microsoft.Extensions.FileProviders.Physical , Microsoft.Extensions.FileProviders.Composite , Microsoft.Extensions.FileProviders.Abstractions , Microsoft.Extensions.DependencyModel , Microsoft.NETCore.App.Runtime.AOT.win-x64.Cross.browser-wasm , Microsoft.NETCore.App.Ref , System.Windows.Extensions , System.Threading.Channels , System.Text.Json , System.Text.Encodings.Web , System.ServiceProcess.ServiceController , System.Drawing.Common , System.DirectoryServices.Protocols , System.Diagnostics.EventLog , System.Diagnostics.DiagnosticSource , System.IO.Pipelines , System.Security.Permissions , System.Security.Cryptography.Xml , System.Security.Cryptography.Pkcs , System.Runtime.CompilerServices.Unsafe , System.Resources.Extensions , System.Reflection.Metadata , System.Net.Http.WinHttpHandler , System.Net.Http.Json
 From Version 6.0.0-rc.1.21420.7 -> To Version 6.0.0-rc.1.21420.15

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>

* Improve Minimal APIs support for request media types #35082 (#35230) (#35579)

* add support for request media types

* [release/6.0-rc1] Update dependencies from dotnet/efcore dotnet/runtime (#35573)

Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Chris R <[email protected]>
Co-authored-by: Safia Abdalla <[email protected]>
Co-authored-by: James Newton-King <[email protected]>
Co-authored-by: Mackinnon Buck <[email protected]>
Co-authored-by: Brennan <[email protected]>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Rafiki Assumani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot cast string to Boolean
3 participants