[FEAT] Add new props to radio button #32
Conversation
SMaxOwok
commented
Mar 31, 2020
SMaxOwok
commented
- Adds labels
- Adds children
- Adds styles
davidmferris
left a comment
davidmferris
left a comment
There was a problem hiding this comment.
Looks really nice. Just one minor comment for you about directory structure of the story.
| import { withA11y } from '@storybook/addon-a11y'; | ||
| import { withKnobs, text, boolean } from '@storybook/addon-knobs'; | ||
|
|
||
| import RadioButton from '../src/RadioButton/RadioButton'; |
There was a problem hiding this comment.
THIS
IS
HUGE
Wanna pull a Rachel and clean up the whole repo?
There was a problem hiding this comment.
Very much, but lemme do that on a separate PR, so this can get merged and I can keep trucking on my actual task...
| /> | ||
|
|
||
| {props.label && ( | ||
| <span className="RadioButton__label">{props.label}</span> |
There was a problem hiding this comment.
I might be missing something, but when should this be used over:
<FormControlLabel
labelHtmlFor="radio"
text="Labeled radio button"
>
<RadioButton id="radio" name="radio" />
</FormControlLabel>
This is another story under "Form Control Label" that looks quite similar to the new Radio button stories
There was a problem hiding this comment.
Ah yeah you're right. I forgot about FormControlLabel.
There was a problem hiding this comment.
Oh, interesting. Let me look at how the radio button changes would work with this as the wrapper.
There was a problem hiding this comment.
So I'm not sure this really works...
The order of the markup has to change for the radio button with children, since the children have to come after the form label. That said, I do think it's valid that RadioButton has its own markup and styles as it's a specific use of a label and not necessarily in line with a generic form label.
Thoughts?
There was a problem hiding this comment.
FWIW when I was doing the form styles I always kinda wondered why we had FormControlLabel and would generally think this would be part of the RadioButton styles. I don't know that flexing and padding are form specific.
There was a problem hiding this comment.
I bit that from MUI. I was really trying to keep the base components as generic as possible so that we wouldn't have to add this label markup for each type of form control.
There was a problem hiding this comment.
This is separate from InputLabel right? I don't know that I fully understand the use case for this vs. adding a label to the radio button. Should this be use the FormControlLabel?
There was a problem hiding this comment.
Yeah InputLabel is meant for like text/number type inputs. FormControlLabel is for radios, switches, and checkboxes (the latter 2 which don't exist yet). Perhaps the naming could use some improvement since it's definitely not obvious
| import { withA11y } from '@storybook/addon-a11y'; | ||
| import { withKnobs, text, boolean } from '@storybook/addon-knobs'; | ||
|
|
||
| import RadioButton from '../src/RadioButton/RadioButton'; |
There was a problem hiding this comment.
THIS
IS
HUGE
Wanna pull a Rachel and clean up the whole repo?
|
|
||
| RadioButton.defaultProps = { | ||
| bordered: false, | ||
| children: null, |
There was a problem hiding this comment.
This works and is typically what I do. Is there a reason to prefer undefined here?
There was a problem hiding this comment.
I think it lines up more with if the prop weren't in this list at all. It seems weird to put a value in here (even if it's null) vs. leaving it with the true default of undefined.
|
|
||
| RadioButton.propTypes = { | ||
| bordered: PropTypes.bool, | ||
| children: PropTypes.node, |
There was a problem hiding this comment.
FWIW per discussions with Darien it's not uncommon to not declare the children propType when you're just going to render it. I think there is a convention that you only declare the children prop when you want to make it required or set some specific rules on it (e.g. only a string) since it's such a weird prop.
There was a problem hiding this comment.
I'm OK with it either way. I think it's useful to know the component can take children, since it's not necessarily obvious a radio button would, but I don't feel strongly. Would you rather it be removed?
There was a problem hiding this comment.
Interesting, for some reason I thought if you rendered children without declaring the prop type you'd get a warning, but if not I'm fine either way.
There was a problem hiding this comment.
I don't think so, though I may be wrong. I don't have a strong opinion here but if no one else cares I'd say let's remove it.
|
|
||
| RadioButton.defaultProps = { | ||
| bordered: false, | ||
| children: null, |
There was a problem hiding this comment.
I think it lines up more with if the prop weren't in this list at all. It seems weird to put a value in here (even if it's null) vs. leaving it with the true default of undefined.
|
|
||
| RadioButton.propTypes = { | ||
| bordered: PropTypes.bool, | ||
| children: PropTypes.node, |
There was a problem hiding this comment.
I don't think so, though I may be wrong. I don't have a strong opinion here but if no one else cares I'd say let's remove it.
