feat(checkbox): Add new component #296
Conversation
Codecov Report
@@ Coverage Diff @@
## master #296 +/- ##
=========================================
- Coverage 97.47% 97.08% -0.4%
=========================================
Files 33 35 +2
Lines 1306 1370 +64
Branches 127 132 +5
=========================================
+ Hits 1273 1330 +57
- Misses 33 40 +7
Continue to review full report at Codecov.
|
| this.setState({classList}); | ||
| }, | ||
| hasNativeControl: () => true, | ||
| isAttachedToDOM: () => true, |
There was a problem hiding this comment.
I see in the core component, it just checks for a parentNode. I'm wondering when this would not be true.
There was a problem hiding this comment.
Can you add a react issue too, linking to #3691
| id={nativeControlId} | ||
| checked={this.state.checked} | ||
| disabled={disabled} | ||
| aria-checked={this.state['aria-checked']} |
packages/checkbox/package.json
Outdated
| @@ -0,0 +1,28 @@ | |||
| { | |||
| "name": "@material/react-checkbox", | |||
| "version": "0.5.0", | |||
packages/checkbox/package.json
Outdated
| }, | ||
| "dependencies": { | ||
| "@material/react-ripple": "^0.5.0", | ||
| "@material/ripple": "^0.40.0", |
There was a problem hiding this comment.
I think you can remove @material/ripple
test/screenshot/checkbox/index.js
Outdated
| import React from 'react'; | ||
| import './index.scss'; | ||
|
|
||
| import Checkbox from '../../../packages/checkbox'; |
There was a problem hiding this comment.
What's the benefit of this? I see that some screenshot tests import /index and some don't
There was a problem hiding this comment.
we should be using /index. it doesn't use the built version in /dist. Its easier to debug and fix issues since we can just update the /index.js file. We also know what were testing against every time (not /dist/index).
test/unit/checkbox/index.test.js
Outdated
| assert.isFalse(wrapper.state().classList.has('test-class-name')); | ||
| }); | ||
|
|
||
| test('#adapter.isChecked returns state.checked', () => { |
There was a problem hiding this comment.
can you add a test for the positive version of this test as well as the indeterminate test below
| test('#adapter.removeNativeControlAttr sets aria-checked state as false', () => { | ||
| const wrapper = shallow(<Checkbox />); | ||
| wrapper.instance().foundation_.adapter_.removeNativeControlAttr('aria-checked'); | ||
| assert.isFalse(wrapper.state()['aria-checked']); |
There was a problem hiding this comment.
I think this is false even without line 106. Add this line above:
wrapper.setState({'aria-checked': true})
test/unit/checkbox/index.test.js
Outdated
| }); | ||
|
|
||
| test('passes nativeControlId to NativeControl through props', () => { | ||
| const wrapper = shallow(<Checkbox nativeControlId={'test-id'}/>); |
There was a problem hiding this comment.
no need to interpolate a string:
nativeControlId='test-id'
test/unit/checkbox/index.test.js
Outdated
| }, | ||
| }; | ||
| wrapper.instance().foundation_.handleChange = td.func(); | ||
| nativeControl.props().onChange(mockEvt); |
There was a problem hiding this comment.
instead of directly calling onChange, use enzyme's simulate method
nativeControl.simulate('change', mockEvt);```
test/unit/checkbox/index.test.js
Outdated
| indeterminate: false, | ||
| }, | ||
| }; | ||
| nativeControl.props().onChange(mockEvt); |
packages/checkbox/index.js
Outdated
| disabled, | ||
| } = this.props; | ||
|
|
||
| if (checked !== prevProps.checked) { |
There was a problem hiding this comment.
this and the below can be put together in one setState call. In order to update indeterminate or checked, you will need to set both, so putting it together in one call is fine.
if (checked !== prevProps.checked || indeterminate !== prevProps.indeterminate) {
this.setState({indeterminate, checked})
...
Then put the setState call into a function, since its shared logic from the onChange method
|
Also I noticed you're not implementing the |
|
|
| <input | ||
| type='checkbox' | ||
| className='mdc-checkbox__native-control' | ||
| ref={rippleActivatorRef} |
There was a problem hiding this comment.
I think we should be doing inputRefs...so if someone passes in a prop, they should be able to programmatically focus a checkbox.focus(). This can be a separate task, especially since we have so many of these NativeX elements. LMK what you think.
packages/checkbox/index.js
Outdated
| aria-checked={this.state['aria-checked']} | ||
| onChange={(evt) => { | ||
| const {checked, indeterminate} = evt.target; | ||
| this.setState({checked, indeterminate}, () => { |
There was a problem hiding this comment.
this and line 64 can be a separate function since its the same logic
Fixes #156