Checkbox updates from responsify checkbox #73
Conversation
| {...other}> | ||
| <FlexItem shrink> | ||
| <input type='checkbox' | ||
| name={name} |
There was a problem hiding this comment.
My information may be out of date, but a good practice used to be making name and id attributes have the same value
There was a problem hiding this comment.
in the case of a set of checkboxes, they all have the same name in a set - so a unique id can't be just the name.
the checkbox can just compose an id out of a name and value and omit the id prop, if thats better than allowing it to pass in an id.
src/Checkbox.jsx
Outdated
|
|
||
| Checkbox.propTypes = { | ||
| checked: React.PropTypes.bool, | ||
| label: React.PropTypes.string, |
There was a problem hiding this comment.
We would ideally make this a shape which can take an element along with string so it can handle translations. For reference you can see https://github.com/meetup/meetup-web-components/blob/master/src/PageHead.jsx#L76
There was a problem hiding this comment.
there is technically a way to get a raw translated string, but I agree that it would be nice to just pass in a <FormattedMessage> element.
src/checkbox.test.jsx
Outdated
|
|
||
| it('should be checked when specified', function() { | ||
| checkbox = TestUtils.renderIntoDocument(<Checkbox name='greeting' id='hello' value='hello' checked />); | ||
| checkboxNode = ReactDOM.findDOMNode(checkbox); |
There was a problem hiding this comment.
We have been having discussions on using findDOMNode in some PR's so not gonna mark this as a change request but... offering this as an alternative approach to writing these tests. You can also read more about this here
it("renders a list in a box with proper CSS classes", function() {
let box = this.TestUtils.findRenderedDOMComponentWithTag(this.component, "div");
expect(box.props.className).toEqual("invitation-list-container");
let list = this.TestUtils.findRenderedDOMComponentWithTag(box, "ul");
expect(list.props.className).toEqual("invitation-list");
});
src/checkbox.story.jsx
Outdated
| storiesOf('Checkbox', module) | ||
| .add('default', () => <Checkbox id='nada' name='no-name' checked={false} />) | ||
| .add('with label', () => <Checkbox label='Ketchup' id='ketchup' checked={false} name='condiment' />) | ||
| .add('checked', () => <Checkbox label='Mustard' checked id='mustard' name='condiment' />); |
There was a problem hiding this comment.
so from what I understand, if this is a controlled component, value or in this case checked always has to be passed in. if I don't I get the controlled vs uncontrolled warning. If there is another way that if checked doesn't have to be specified as a prop if false, let me know.
There was a problem hiding this comment.
also TODO: make a list of checkboxes to demo in storybook.
| {...other}> | ||
| <FlexItem shrink> | ||
| <input type='checkbox' | ||
| name={name} |
There was a problem hiding this comment.
in the case of a set of checkboxes, they all have the same name in a set - so a unique id can't be just the name.
the checkbox can just compose an id out of a name and value and omit the id prop, if thats better than allowing it to pass in an id.
src/Checkbox.jsx
Outdated
| /> | ||
| </FlexItem> | ||
| <FlexItem> | ||
| <label className='label--minor' htmlFor={elId}>{label}</label> |
There was a problem hiding this comment.
TL;DR: Checkbox labels shouldn't be bold
The label--minor class was something we were playing with to "un-bold" <label> for radio and checkbox inputs. We wanted to use the bold text for <legend> when we have a set of checkboxes that need to be labeled.
E.g.: ```
Colors
Red
Blue ```
The CSS behind label--minor didn't make it into this repo, it should probably go in swarm-sasstools
There was a problem hiding this comment.
ok. what class do we prefer?
There was a problem hiding this comment.
Go ahead and use label--minor in this component; I'll make sure the class will make it into swarm-sasstools with this PR: #80
This has been filed in Jira as: https://meetup.atlassian.net/browse/SDS-157
Once swarm-sasstools is merged into this repo, the class will be defined and your labels will get prettier.
src/checkbox.test.jsx
Outdated
|
|
||
| it('exists', function() { | ||
| expect(checkbox).not.toBeNull(); | ||
| }); |
There was a problem hiding this comment.
because your using findRenderedDOMComponent this wont return null when it fails, instead it throws an error. i would do
expect(() => TestUtils.findRenderedDOMComponentWithType(checkboxComponent, Checkbox)).not.toThrow()
instead
Related issues
Fixes https://meetup.atlassian.net/browse/MUP-7777
Description
Checkbox component for Event create. I pulled the initial files out of responsify and made some changes. Tests to come.
Screenshots (if applicable)