-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[added] RadioButton and RadioGroup #962
Conversation
This also might against the wrong branch... |
|
||
return ( | ||
<label className={classNames(classes)}> | ||
{input} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the label should probably also add htmlFor
if there is an id prop, for better accessibility. We might also want to add id
as a requiredForA11y
prop to encourage good behavior. I wouldn't assume the prop is there tho to avoid breaking compatibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I initially didn't implement it since the wrapping automatically associated the radio button with the label and gave me the green light through the WAVE tool. Having the id would certainly be a good enhancement, though I don't think I'll be making it a required prop.
af2d07f
to
dd8f75b
Compare
@jquense Got the |
Before we pull this in, how do we want to go about handling a default value? |
The pattern I've seen is to provide an |
As described here as the |
9afd2d9
to
da5fbef
Compare
I think the case of controlled/uncontrolled inputs |
👍 |
👍 |
e747b62
to
1d21e60
Compare
<label className={classNames(classes)} htmlFor={id}> | ||
{input} | ||
{label} | ||
</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id
and label
vars from 26 line
const {id, label} = this.props;
are not used anywhere else
why don't just ?
<label className={classNames(classes)} htmlFor={this.props.id}>
{input}
{this.props.label}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks cleaner to me this way, and babel will transform it to this.props.id
anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the clarification 🍒
no problem at all with it. it is just a matter of personal taste 😉
I think having empty bool values is fine for now. It might be better to just solve this in the prop documentation code with like a @aabenoja One thing I might do tho is name the noop function used as the default so it looks clearer in the docs. let noop = ()=>{}
defaultProps = {
onChange: noop
} up to you |
@AlexKVal I don't see anything necessarily wrong with @jquense I agree, that's something we could do during prop documentation generation. I could sneak that in with this PR. As for the |
Agree. |
A potential problem with filling in a default of e.g. |
@taion I've been swamped for a while and haven't been able to set aside the time, unfortunately. |
No worries - going to push this down one more milestone then. Probably better all-around since we have quite a few things for v0.25 already. |
…d RadioGroup components
5879b46
to
b758414
Compare
Is this essentially good to go now, except for
? |
It seems we should decide the way to go: #962 (comment)
Wrap it or no ? |
Sorry, I've been swamped with other things on my end. In all honestly, using |
Not the button I wanted to hit >.> |
@@ -0,0 +1,39 @@ | |||
const SimpleRadioGroup = React.createClass({ | |||
getInitialState() { | |||
return {message: undefined}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think return {}
shoud be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
({message: undefined}).message=== ({}).message
returns true
@bbenezech At this point, our plan is to revise the |
@taion I see. Sounds a bit though for a first commit :/ |
Closing this for now – will incorporate this into the form/input rewrite. |
We're just not going to do it for now – if you want something like a radio group, consider a higher-level abstraction like React Formal. For now, we don't see much value in going beyond matching the TWBS API in a thin way here. |
Since there is no way to add some radio buttons that toggle a single value and then read the corresponding value with uncontrolled props ( without relying on state) i think there is value in here. |
Part of #342.
The
RadioGroup
might also resolve #365 by keeping track of the selected item.