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

[RadioButtonGroup] breaks when nesting non-RadioButton components #2225

Closed
bshyong opened this issue Nov 20, 2015 · 24 comments
Closed

[RadioButtonGroup] breaks when nesting non-RadioButton components #2225

bshyong opened this issue Nov 20, 2015 · 24 comments
Labels
component: radio This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation new feature New feature or request priority: important This change can make a difference

Comments

@bshyong
Copy link

bshyong commented Nov 20, 2015

I'm trying to do some simple styling. The following example fails.

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

I have written a patch that resolves this; let me know if I should issue a pull request.

@oliviertassinari

This comment has been minimized.

@bshyong
Copy link
Author

bshyong commented Nov 20, 2015

Olivier, I need to nest the buttons in a div with clearfix for the layout that I want. If you look at the internal implementation of RadioButtonGroup it Iterates through and adds props to RadioButtons - I don't see a reason for it to nuke your layout structure or for the component to break if you have a nonRadioButton element nested inside. Thoughts?

@oliviertassinari

This comment has been minimized.

@bshyong

This comment has been minimized.

@alitaheri alitaheri added the bug 🐛 Something doesn't work label Dec 9, 2015
@suricactus
Copy link

Hey, any progress on this? I ended up with the same bug, using react-flexgrid components.

bshyong added a commit to bshyong/material-ui that referenced this issue Mar 7, 2016
@bshyong
Copy link
Author

bshyong commented Mar 7, 2016

PR here #3627

@rkmax
Copy link
Contributor

rkmax commented Apr 5, 2016

I'm having troubles also for this. because I need render a radios and text fields together. any progress?

@bshyong

This comment has been minimized.

@RohovDmytro

This comment has been minimized.

@oliviertassinari oliviertassinari changed the title RadioButtonGroup breaks when nesting non-RadioButton components [RadioButtonGroup] breaks when nesting non-RadioButton components Jan 1, 2017
@coreyar
Copy link

coreyar commented Mar 22, 2017

The solution @bshyong wrote works great.

You will need to update line 112 to read option.type.name === 'RadioButton'.

@bshyong
Copy link
Author

bshyong commented Mar 22, 2017

If possible, someone can reopen and merge this fix to resolve this issue: #3627

@mbrookes
Copy link
Member

mbrookes commented Mar 22, 2017

@bshyong #3627 (comment). However, focus has moved to next. I would suggest waiting for @oliviertassinari's input on whether a revised PR would be accepted before doing any further work on it.

@bshyong
Copy link
Author

bshyong commented Mar 22, 2017

Makes sense. I would stick with the recursive approach for now, as I would like to avoid the use of context unless it is absolutely necessary. https://facebook.github.io/react/docs/context.html#why-not-to-use-context

We can revisit if this is still a problem once next is complete

@mbrookes
Copy link
Member

I would like to avoid the use of context

I'm very familiar with that page, but I think this use-case sits outside that warning.

From a brief look at the code, it does seem next will have a similar issue. Whether addressing it in the library is appropriate or should be handled by the dev is another question. It's a simple enough matter to make the wrapper component pass props to the RadioButton.

@car3oon
Copy link

car3oon commented Jun 6, 2017

Hi guys
Any progress on merging this issue?

@wcandillon
Copy link
Contributor

I'm also interested to see this feature shipped.

@oliviertassinari oliviertassinari added new feature New feature or request v1 and removed bug 🐛 Something doesn't work labels Jul 28, 2017
@marutha
Copy link

marutha commented Sep 18, 2017

I am also interested to see this

@thiagoxvo
Copy link

Same here...

@fabyeah
Copy link

fabyeah commented Oct 16, 2017

I need this to display additional content below the selected radio button.

@bshyong
Copy link
Author

bshyong commented Oct 16, 2017

@fabyeah my PR hasn't been merged into master but you can take a look here #3627

You could copy the RadioButtonGroup.js component from my repo and try it out for your own use (keeping everything else the same)

@mui mui deleted a comment from robinclaes Nov 17, 2017
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 9, 2017

In order to keep the implementation simple and slim, I'm closing the issue with the following solution.
On the v1 branch, the canonical way of using the RadioButtonGroup component is:

<RadioGroup
  aria-label="gender"
  name="gender2"
  className={classes.group}
  value={this.state.value}
  onChange={this.handleChange}
>
  <Radio value="male" />
  <Radio value="female" />
  <Radio value="other" />
  <Radio value="disabled" disabled />
</RadioGroup>

capture d ecran 2017-12-09 a 16 05 19

Still, one might want to change the way the radios are rendered. We have the FormControlLabel component for this use-case:

<RadioGroup
  aria-label="gender"
  name="gender2"
  className={classes.group}
  value={this.state.value}
  onChange={this.handleChange}
>
  <FormControlLabel value="male" control={<Radio />} label="Male" />
  <FormControlLabel value="female" control={<Radio />} label="Female" />
  <FormControlLabel value="other" control={<Radio />} label="Other" />
  <FormControlLabel value="disabled" disabled control={<Radio />} label="Disabled" />
</RadioGroup>

capture d ecran 2017-12-09 a 16 05 34

So, either you can get along with the FormControlLabel component or you can start from the FormControlLabel implementation to build your own. Let me know if it's no enough.

@oliviertassinari oliviertassinari added support: question Community support but can be turned into an improvement component: radio This is the name of the generic UI component, not the React module! labels Dec 9, 2017
@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Mar 9, 2018
@oliviertassinari
Copy link
Member

I'm reopening as #10576 makes me believe we should be documenting this behavior and add an example of how to forward the properties.

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 3, 2019

Workaround n°1.

The RadioGroup component is using the React cloneElement API: #12921. This is a problem because it's hidden from our users, it's happening behind the scene. The immediate children of the RadioGroup should handle the injected properties. If they don't, the control is lost.
Now, you can expose the injecteed properties with a Wire component and apply them where needed:

const Wire = ({ children, ...props }) => children(props);

// …

<RadioGroup
  aria-label="Gender"
  name="gender1"
  className={classes.group}
  value={this.state.value}
  onChange={this.handleChange}
>
  <Wire value="female">
    {props => (
      <Tooltip title="text">
        <FormControlLabel
          control={<Radio />}
          label="Female"
          {...props}
        />
      </Tooltip>
    )}
  </Wire>

https://codesandbox.io/s/zr7x1xx32x

Workaround n°2.

Form libraries like react-final-from can handle the state centralization problem, you can choose not to delegate this task to the RadioGroup. By handling each Radio independently, you can workaround the problem.

@onpaws
Copy link
Contributor

onpaws commented Mar 8, 2019

Thanks Oliver the <Wire> component appears to be working for my needs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: radio This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation new feature New feature or request priority: important This change can make a difference
Projects
None yet
Development

No branches or pull requests