[RadioButtonGroup] Enable handling of non-RadioButton children

[bshyong]

When nesting children that are not RadioButtons in the RadioButtonGroup, the non-RadioButton elements are ignored.

The following example generates unexpected behavior. All non-RadioButton components are ignored.

Source code:

<RadioButtonGroup>
  <div className="col6">
    <RadioButton />
  </div>
  <div className="col6">
    <RadioButton />
  </div>
</RadioButtonGroup>

Rendered output:

<RadioButtonGroup>
    <RadioButton />
    <RadioButton />
</RadioButtonGroup>

Resolves #2225

[alitaheri]

this._radioButtonMapFunc

or rather, rename _radioButtonMapFunc to radioButtonMapFunc

[mbrookes]

@bshyong Please could you also update the commit title to match the PR title? (Helps keep the commit history clean).

[bshyong]

Hi guys, thank you for reviewing! I've made the requested changes.

[alitaheri]

This does fix the issue for now. But I think this should be contextual to avoid recursive calls on each render. recursion is heavy stuff. But I'm still ok with it as there won't usually be a lot of nested stuff in a radio button group. 👍

@callemall/material-ui Guys take a look please 👍

[mbrookes]

@alitaheri

I think this should be contextual to avoid recursive calls on each render

Could you explain? Thanks.

[alitaheri]

@mbrookes What I mean is, this implementation recursively walks down the tree to find the RadioButtons which is much more expensive than providing a context with RadioButtonGroup and having it passed down to RadioButton so that they can register themselves as members of that group.

[mbrookes]

@alitaheri Thanks. 👍 @bshyong Are you happy to update this PR with that approach?

[bshyong]

Hey guys,

I just looked through the code; I'm not sure I understand the context
approach. It seems to me that we need to access every child anyways to
preserve the structure. React.Children.map only maps over immediate
children so I wrote a map function that looks for RadioButtons within
non-RadioButton children and renders them out as well.

Wonder if I am missing something here or if one of you can provide some
pseudo code to better illustrate using a context?

Ben Shyong
909-274-9664

On Sat, May 7, 2016 at 7:04 AM, Matt Brookes notifications@github.com
wrote:

@alitaheri https://github.com/alitaheri Thanks. 👍 @bshyong
https://github.com/bshyong Are you happy to update this PR with that
approach?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#3627 (comment)

[alitaheri]

@bshyong Well the idea is, we reverse the flow of control. instead of RadioButtonGroup looking for it's RadioButtons, the RadioButtons register themselves via the hook e.g. this.context.registerRadioButton(this, value) which will then put this RadioButton into the list of RadioButtons that that group controls. If that still doesn't make sense, then maybe this approach is over engineering, and must be avoided for the sake of maintainability. But this solution performs better, though that might be irrelevant since not so many items would show up inside a group anyway.

[bshyong]

That sounds like a clever approach. I'll need to find some time to look at
this again to see how to implement this.

Ben Shyong
909-274-9664

On Sat, May 7, 2016 at 10:43 AM, Ali Taheri Moghaddar <
notifications@github.com> wrote:

@bshyong https://github.com/bshyong Well the idea is, we reverse the
flow of control. instead of RadioButtonGroup looking for it's
RadioButtons, the RadioButtons register themselves via the hook e.g. this.context.registerRadioButton(this,
value) which will then put this RadioButton into the list of RadioButtons
that that group controls. If that still doesn't make sense, then maybe this
approach is over engineering, and must be avoided for the sake of
maintainability. But this solution performs better, though that might be
irrelevant since not so many items would show up inside a group anyway.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#3627 (comment)

[nathanmarks]

Closing this as there is no movement.