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

InputRadio component with form support #23415

Merged
merged 15 commits into from
Jul 2, 2020
Merged

Conversation

MackinnonBuck
Copy link
Member

@MackinnonBuck MackinnonBuck commented Jun 26, 2020

Summary:

  • Implemented a new InputRadio component with form support.
  • Added E2E tests to validate its interactivity with edit context.
  • Added tests to verify that InputRadio components interact with InputRadioGroup components as expected.
  • Added int and Guid binding support for InputRadio and InputSelect.

Usage example:

<InputRadioGroup Name="optional-name" @bind-value="someBoundValue">
    <InputRadio Name="optional-name" Value="option1">
    <InputRadio Name="optional-name" Value="option2">
</InputRadioGroup>

If Name is omitted, InputRadio components are grouped by their most recent ancestor.

Addresses #5579, #9939 and #19562.

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Jun 26, 2020
@MackinnonBuck MackinnonBuck requested a review from pranavkm June 26, 2020 22:35
@MackinnonBuck
Copy link
Member Author

MackinnonBuck commented Jun 26, 2020

Open question: Should we ensure that the type of the value bound to InputRadio is either a string or Enum so that we can be sure it will be formatted correctly?

Edit: I'm going to proceed under the assumption that we should, given InputSelect is implemented this way.

InputChoice contains checks for valid choice types that used to exist in InputSelect. Both InputSelect and InputRadio now derive from InputChoice and thus also contain those checks.
@MackinnonBuck MackinnonBuck requested a review from javiercn June 29, 2020 20:51
src/Components/Web/src/Forms/InputRadio.cs Outdated Show resolved Hide resolved
src/Components/Web/src/Forms/InputRadio.cs Outdated Show resolved Hide resolved
src/Components/Web/src/Forms/InputRadioGroup.cs Outdated Show resolved Hide resolved
src/Components/Web/src/Forms/InputChoice.cs Outdated Show resolved Hide resolved
src/Components/Web/src/Forms/InputRadio.cs Outdated Show resolved Hide resolved
src/Components/Web/src/Forms/InputRadioGroup.cs Outdated Show resolved Hide resolved
src/Components/Web/test/Forms/InputRadioTest.cs Outdated Show resolved Hide resolved
src/Components/Web/test/Forms/InputRadioTest.cs Outdated Show resolved Hide resolved
src/Components/Web/src/Forms/InputBase.cs Show resolved Hide resolved
@pranavkm pranavkm added this to the 5.0.0-preview8 milestone Jun 29, 2020
@MackinnonBuck MackinnonBuck requested a review from pranavkm June 29, 2020 23:18
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.

Looks great. @dotnet/aspnet-blazor-eng could you have a look please?

src/Components/Web/src/Forms/InputExtensions.cs Outdated Show resolved Hide resolved
@MackinnonBuck MackinnonBuck requested a review from javiercn June 30, 2020 17:24
@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Jul 1, 2020

This is really great, @MackinnonBuck! Thanks for figuring out so many of the tricky details (e.g., around parsing) here and building it in a clean way.

One suggestion around the API: I wonder if it might be possible to make this even more closely equivalent to <InputSelect>, and with fewer options for developers to get things wrong, by being a bit more restrictive around the usage pattern. Specifically, what if we required <InputRadioGroup>, and then put the bind-Value there? Example:

<InputRadioGroup @bind-Value="mySelection">
    <InputRadio Value="Colors.Red">A warm red</InputRadio>
    <InputRadio Value="Colors.Green">An earthy green</InputRadio>
    <InputRadio Value="Colors.Blue">A corporate blue</InputRadio>
</InputRadioGroup>

With this, you no longer have to repeat @bind-Value="mySelection", nor do you have to understand the difference between @bind-Value and SelectedValue. It makes it pretty much a pure swap-in equivalent replacement for <InputSelect>.

Your scheme around an optional "name" looks like it's sufficient to support even the most unusual UI shapes alongside the requirement to declare a group explicitly, e.g. for this:

Pick one color and one country:

[ ] Red
[x] Japan
[ ] Green
[ ] Yemen
[ ] Blue
[ ] Latvia
[x] Orange

... developers could do:

<InputRadioGroup Name="country" @bind-Value="selectedCountry">
    <InputRadioGroup Name="color" @bind-Value="selectedColor">
        <InputRadio Name="color" Value="red" />
        <InputRadio Name="country" Value="japan" />
        <InputRadio Name="color" Value="green" />
        <InputRadio Name="country" Value="yemen" />
        <InputRadio Name="color" Value="blue" />
        <InputRadio Name="country" Value="latvia" />
        <InputRadio Name="color" Value="orange" />
    </InputRadioGroup>
</InputRadioGroup>

BTW if the nested InputRadioGroup turns out to be tricky, I think it would be fine to either not support that initially (it is an obscure scenario), or I'd be happy to work with you on suggesting a mechanism for implementing it.

@MackinnonBuck
Copy link
Member Author

@SteveSandersonMS Thanks for the great suggestions!

I have made those improvements and added tests for them. I had to introduce InputRadioContext and use it as the cascading value to solve issues with TValue mismatching between InputRadioGroup and its child InputRadio components when Nullable was involved. It also helped with making nested radio groups work, as you described in your last comment. Please let me know if you think this approach is desirable. 😄

@MackinnonBuck
Copy link
Member Author

Is there a particular reason we ever limited what types could be bound when using InputSelect? As I was adding Guid and int support, I noticed my code started to look similar to

public static BindFormatter<T> Get<T>()

If I modify InputSelect and InputRadioGroup to use BindConverter.TryConvertTo<T> instead of the existing if/else chain, all existing functionality is preserved, and we get Guid and int binding support for free (I've added tests to verify this). This would resolve #9939 and #19562.

Copy link

@mrpmorris mrpmorris left a comment

Choose a reason for hiding this comment

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

Very good :)

@SteveSandersonMS
Copy link
Member

This is totally superb! Thanks for making the updates. I did have one further suggested tweak about where the validation CSS classes should show up - let me know what you think. Hopefully that would be a very small change, but if you think it's not the right design please say so. I'd say this is ready to merge once that last question is resolved. The test cases look great too.

Is there a particular reason we ever limited what types could be bound when using InputSelect?

Not as far as I know. It's quite possible that it's a historical artifact of what order we implemented things in. It's great you've found that BindConverter.TryConvertTo<T> works well here - that seems like a big win!

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.

Brilliant, thanks for the final updates! This is a great feature.

@ehills
Copy link

ehills commented Jan 25, 2021

Hi team, i've noticed that it's not possible to add an onchange event listener using InputRadio/InputRadioGroup.
It appears that the 'onchange' slot of the InputRadio essentially gets used up by the magic that links the RadioGroup.
Is there another work around?
i.e. I want to fire a onchange event to update a parent element in the form once a radio button has been selected.

@ghost
Copy link

ghost commented Jan 25, 2021

Hi @ehills. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

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.

6 participants