Skip to content

Make InputRadioGroup preserve child elements#40148

Merged
SteveSandersonMS merged 3 commits into
mainfrom
stevesa/inputradiogroup-fix
Feb 14, 2022
Merged

Make InputRadioGroup preserve child elements#40148
SteveSandersonMS merged 3 commits into
mainfrom
stevesa/inputradiogroup-fix

Conversation

@SteveSandersonMS
Copy link
Copy Markdown
Member

@SteveSandersonMS SteveSandersonMS commented Feb 11, 2022

Fixes #31610 and #39145, and supersedes #39208

@MariovanZeist - thanks so much for contributing #39208. I hope you don't mind me coming up with a slightly different solution in this PR. The reasons I wanted to do it differently are:

  • The approach in Fix: InputRadioGroup not registering initial click in some circumstances #39208 is to create a custom system of registering for notification to be re-rendered when an ancestor changes. This happens to be equivalent to what CascadingValue does natively when IsFixed is false, so simply by removing the IsFixed=true we get the desired behavior without needing a new mechanism.
  • There isn't really any perf drawback to removing the IsFixed=true. Descendants have to be notified to re-render one way or another, because their actual output must change if they become (de)selected. Prior to this fix, the mechanism used @key to recreate the entire child hierarchy every time, whereas now it retains the child hierarchy and just triggers the appropriate re-rendering.

The fix could have been reduced to removing these two lines from InputRadioGroup:

        builder.SetKey(_context);
        builder.AddAttribute(1, "IsFixed", true);

However, in this PR I also chose to reduce allocations by not creating a new InputRadioContext on every render, and instead retaining the existing instance and mutating it in place. It's not particularly complicated to do that but did add quite a few extra changes to the PR diff.

As far as I know, this produces the desired behavior and works as well as the PR at #39208. I verified that it also passes the E2E tests supplied in that PR. But @MariovanZeist, if you have any concerns that the approach here doesn't meet your goals, please let me know!

@SteveSandersonMS SteveSandersonMS added the area-blazor Includes: Blazor, Razor Components label Feb 11, 2022
@SteveSandersonMS SteveSandersonMS requested a review from a team February 11, 2022 17:29
internal static class AttributeUtilities
{
public static string CombineClassNames(IReadOnlyDictionary<string, object>? additionalAttributes, string classNames)
public static string? CombineClassNames(IReadOnlyDictionary<string, object>? additionalAttributes, string? classNames)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The nullability changes here and below are just because this code actually does work fine with a null classNames, and the new code benefits by not having to do a ?? string.Empty.

Comment thread src/Components/Web/src/Forms/InputRadioContext.cs Outdated
{
var groupName = !string.IsNullOrEmpty(Name) ? Name : _defaultGroupName;
var fieldClass = EditContext?.FieldCssClass(FieldIdentifier) ?? string.Empty;
var changeEventCallback = EventCallback.Factory.CreateBinder<string?>(this, __value => CurrentValueAsString = __value, CurrentValueAsString);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We can also avoid creating a new delegate here on every render.

Comment thread src/Components/Web/src/Forms/InputRadioContext.cs Outdated
Comment thread src/Components/Web/src/Forms/InputRadioGroup.cs
@MariovanZeist
Copy link
Copy Markdown
Contributor

@SteveSandersonMS I prefer this solution over mine, as it's cleaner
I didn't know of this particular "side effect" of having IsFixed set to false
I guess due to:

if (_subscribers != null && ChangeDetection.MayHaveChanged(previousValue, Value))
{
NotifySubscribers(parameters.Lifetime);
}

Always good to learn something new.

All concerns addressed in my PR are also addressed here

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.

InputRadioGroup does not support state preservation for child content

4 participants