fixes from CR

Adslot · May 11, 2018 · 0f5dae4 · 0f5dae4
1 parent 423bb46
commit 0f5dae4
Show file tree

Hide file tree

Showing 7 changed files with 85 additions and 37 deletions.
diff --git a/docs/examples/CheckboxSingleExample.jsx b/docs/examples/CheckboxSingleExample.jsx
@@ -49,6 +49,7 @@ const exampleProps = {
         {
           propType: 'value',
           type: 'string',
+          note: 'Required.',
         },
         {
           propType: 'checked',

diff --git a/src/components/adslot-ui/CheckboxGroup/index.jsx b/src/components/adslot-ui/CheckboxGroup/index.jsx
@@ -1,9 +1,12 @@
 import React from 'react';
 import PropTypes from 'prop-types';
 
-import './styles.scss';
-
 class CheckboxGroup extends React.Component {
+  static getDerivedStateFromProps(newProps) {
+    if (newProps.value) return { value: newProps.value };
+    return false;
+  }
+
   constructor(props) {
     super(props);
     this.state = {
@@ -43,7 +46,7 @@ class CheckboxGroup extends React.Component {
   }
 
   render() {
-    const classes = `checkbox-group ${this.props.customClasses}`;
+    const classes = `checkbox-group ${this.props.className}`;
     return <div className={classes}>{this.renderChildren()}</div>;
   }
 }
@@ -52,7 +55,7 @@ CheckboxGroup.propTypes = {
   name: PropTypes.string.isRequired,
   value: PropTypes.arrayOf(PropTypes.string),
   children: PropTypes.arrayOf(PropTypes.element).isRequired,
-  customClasses: PropTypes.string,
+  className: PropTypes.string,
   onChange: PropTypes.func,
 };
 

diff --git a/src/components/adslot-ui/CheckboxGroup/index.spec.jsx b/src/components/adslot-ui/CheckboxGroup/index.spec.jsx
@@ -1,11 +1,26 @@
 import React from 'react';
 import sinon from 'sinon';
-import { shallow } from 'enzyme';
+import { shallow, mount } from 'enzyme';
 import { CheckboxSingle } from 'adslot-ui';
 import CheckboxGroup from '.';
 
 describe('CheckboxGroup', () => {
-  it('should render correctly and handle checkbox change events', () => {
+  it('should render with props', () => {
+    const component = shallow(
+      <CheckboxGroup name="movies" value={['terminator', 'predator']} className="custom-class">
+        <CheckboxSingle label="The Terminator" value="terminator" />
+        <CheckboxSingle label="Predator" value="predator" />
+        <CheckboxSingle label="The Sound of Music" value="soundofmusic" />
+      </CheckboxGroup>
+    );
+
+    expect(component.hasClass('custom-class')).to.equal(true);
+    const childCheckboxes = component.find('CheckboxSingle');
+    expect(childCheckboxes.length).to.equal(3);
+    expect(component.state().value).to.eql(['terminator', 'predator']);
+  });
+
+  it('should handle checkbox change events', () => {
     const onChangeGroup = sinon.spy();
     const onChangeIndividual = sinon.spy();
     const component = shallow(
@@ -17,8 +32,6 @@ describe('CheckboxGroup', () => {
     );
 
     const childCheckboxes = component.find('CheckboxSingle');
-    expect(childCheckboxes.length).to.equal(3);
-
     const firstChild = childCheckboxes.at(0);
     const event = { value: 'terminator', checked: false };
     firstChild.simulate('change', event);
@@ -40,7 +53,7 @@ describe('CheckboxGroup', () => {
     expect(childCheckboxes.length).to.equal(3);
   });
 
-  it('should render without onChange or value props', () => {
+  it('should handle change events without a custom onChange handler', () => {
     const event = { value: 'terminator', checked: true };
     const component = shallow(
       <CheckboxGroup name="movies">
@@ -51,5 +64,18 @@ describe('CheckboxGroup', () => {
     );
     const firstChild = component.find('CheckboxSingle').at(0);
     firstChild.simulate('change', event);
+    expect(component.state().value).to.eql(['terminator']);
+  });
+
+  it('should handle props changes', () => {
+    const component = mount(
+      <CheckboxGroup name="movies" value={['terminator', 'predator']} className="custom-class">
+        <CheckboxSingle label="The Terminator" value="terminator" />
+        <CheckboxSingle label="Predator" value="predator" />
+        <CheckboxSingle label="The Sound of Music" value="soundofmusic" />
+      </CheckboxGroup>
+    );
+    component.setProps({ value: ['terminator', 'soundofmusic'] });
+    expect(component.state().value).to.eql(['terminator', 'soundofmusic']);
   });
 });
diff --git a/src/components/adslot-ui/CheckboxGroup/styles.scss b/src/components/adslot-ui/CheckboxGroup/styles.scss
diff --git a/src/components/adslot-ui/CheckboxSingle/index.jsx b/src/components/adslot-ui/CheckboxSingle/index.jsx
@@ -1,10 +1,20 @@
+import _ from 'lodash';
 import React from 'react';
 import PropTypes from 'prop-types';
 import { expandDts } from 'lib/utils';
 
 import './styles.scss';
 
 class CheckboxSingle extends React.Component {
+  static getDerivedStateFromProps(newProps, prevState) {
+    const isChecked =  _.isUndefined(newProps.checked)? prevState.checked : newProps.checked;
+    const isDisabled =  _.isUndefined(newProps.disabled)? prevState.disabled : newProps.disabled;
+    return {
+      checked: isChecked,
+      disabled: isDisabled,
+    };
+  }
+
   constructor(props) {
     super(props);
     this.state = {
@@ -31,15 +41,6 @@ class CheckboxSingle extends React.Component {
 
   render() {
     const { name, value, label, dts } = this.props;
-    const onChangeHandler = this.onChangeDefault;
-    const booleanAttributes = { disabled: '', checked: '' };
-    if (this.state.disabled) {
-      booleanAttributes.disabled = 'disabled';
-    }
-    if (this.state.checked) {
-      booleanAttributes.checked = 'checked';
-    }
-
     if (label) {
       return (
         <div className="checkbox-single">
@@ -48,8 +49,9 @@ class CheckboxSingle extends React.Component {
               type="checkbox"
               name={name}
               value={value}
-              onChange={onChangeHandler}
-              {...booleanAttributes}
+              onChange={this.onChangeDefault}
+              disabled={this.state.disabled}
+              checked={this.state.checked}
               {...expandDts(dts)}
             />
             <span>{label}</span>
@@ -63,8 +65,9 @@ class CheckboxSingle extends React.Component {
           type="checkbox"
           name={name}
           value={value}
-          onChange={onChangeHandler}
-          {...booleanAttributes}
+          onChange={this.onChangeDefault}
+          disabled={this.state.disabled}
+          checked={this.state.checked}
           {...expandDts(dts)}
         />
       </div>

diff --git a/src/components/adslot-ui/CheckboxSingle/index.spec.jsx b/src/components/adslot-ui/CheckboxSingle/index.spec.jsx
@@ -1,10 +1,24 @@
 import React from 'react';
-import { shallow } from 'enzyme';
+import { shallow, mount } from 'enzyme';
 import sinon from 'sinon';
 import CheckboxSingle from '.';
 
 describe('CheckboxSingle', () => {
-  it('should render correctly and handle change event', () => {
+  it('should render with props', () => {
+    const component = shallow(
+      <CheckboxSingle label="The Terminator" name="movies" value="terminator" dts="checkbox-terminator" />
+    );
+    const checkboxElement = component.find('input[type="checkbox"]');
+    const labelElement = component.find('label');
+    expect(labelElement.text()).to.equal('The Terminator');
+    expect(checkboxElement).to.have.length(1);
+    expect(checkboxElement.prop('name')).to.equal('movies');
+    expect(checkboxElement.prop('value')).to.equal('terminator');
+    expect(checkboxElement.prop('checked')).to.equal(false);
+    expect(checkboxElement.prop('data-test-selector')).to.equal('checkbox-terminator');
+  });
+
+  it('should handle change event', () => {
     const onChangeHandler = sinon.spy();
     const component = shallow(
       <CheckboxSingle
@@ -16,16 +30,10 @@ describe('CheckboxSingle', () => {
       />
     );
     const checkboxElement = component.find('input[type="checkbox"]');
-    const labelElement = component.find('label');
-    expect(labelElement.text()).to.equal('The Terminator');
-    expect(checkboxElement).to.have.length(1);
-    expect(checkboxElement.prop('name')).to.equal('movies');
-    expect(checkboxElement.prop('value')).to.equal('terminator');
-    expect(checkboxElement.prop('checked')).to.equal('');
-    expect(checkboxElement.prop('data-test-selector')).to.equal('checkbox-terminator');
-    const event = { target: { name: 'movies', value: 'terminator' } };
+    const event = { target: { checked: true, disabled: true } };
     checkboxElement.simulate('change', event);
     expect(onChangeHandler.callCount).to.equal(1);
+    expect(component.state()).to.eql({ checked: true, disabled: true });
   });
 
   it('should render without a label', () => {
@@ -49,10 +57,19 @@ describe('CheckboxSingle', () => {
     expect(component.state()).to.eql({ checked: true, disabled: true });
   });
 
-  it('should render without a custom onChange handler', () => {
-    const component = shallow(<CheckboxSingle name="movies" value="terminator" />);
+  it('should handle change events without a custom onChange handler', () => {
+    const onChangeHandler = sinon.spy();
+    const component = shallow(<CheckboxSingle name="movies" value="terminator" onChange={onChangeHandler} />);
     const checkboxElement = component.find('input[type="checkbox"]');
-    const event = { target: { name: 'movies', value: 'terminator' } };
+    const event = { target: { checked: true, disabled: false } };
     checkboxElement.simulate('change', event);
+    expect(onChangeHandler.callCount).to.equal(1);
+    expect(component.state()).to.eql({ checked: true, disabled: false });
+  });
+
+  it('should handle props changes', () => {
+    const component = mount(<CheckboxSingle name="movies" value="terminator" />);
+    component.setProps({ checked: true, disabled: true });
+    expect(component.state()).to.eql({ checked: true, disabled: true });
   });
 });
diff --git a/src/components/adslot-ui/CheckboxSingle/styles.scss b/src/components/adslot-ui/CheckboxSingle/styles.scss
@@ -13,5 +13,3 @@
     display: inline-block;
   }
 }
-
-