Merge pull request #129 from appfolio/improveErrors

Improve errors

src/components/BoundForm.js

@@ -19,6 +19,12 @@ class BoundForm extends React.Component {
     onChange: noop
   };
 
+  static childContextTypes = {
+    value: React.PropTypes.object,
+    errors: React.PropTypes.object,
+    onChange: React.PropTypes.func
+  }
+
   constructor(props) {
     super(props);
 
@@ -27,6 +33,14 @@ class BoundForm extends React.Component {
     };
   }
 
+  getChildContext() {
+    return {
+      value: this.state.formData,
+      errors: this.props.errors,
+      onChange: this.handleChange
+    }
+  }
+
   onSubmit = e => {
     e.preventDefault();
     this.props.onSubmit(e, this.state.formData);
@@ -39,23 +53,8 @@ class BoundForm extends React.Component {
   }
 
   render() {
-    const children = React.Children.map(this.props.children, child => {
-      if (child.type !== FormRow) { return child; } // TODO make this better, using Context?
-
-      const value = this.state.formData[child.props.name] || '';
-      const feedback = this.props.errors[child.props.name];
-      const color = feedback ? 'danger' : null;
-
-      return React.cloneElement(child, {
-        feedback,
-        color,
-        value,
-        onChange: this.handleChange(child.props.name)
-      });
-    });
-
     return (
-      <Form children={children} onSubmit={this.onSubmit} />
+      <Form children={this.props.children} onSubmit={this.onSubmit} />
     );
   }
 }

src/components/BoundFormRow.js

@@ -0,0 +1,23 @@
+import React from 'react';
+import FormRow from './FormRow';
+
+const BoundFormRow = (props, { value = {}, errors = {}, onChange }) => (
+  <FormRow
+    value={value[props.name] || ''}
+    feedback={errors[props.name] || ''}
+    onChange={onChange(props.name)}
+    {...props}
+  />
+);
+
+BoundFormRow.contextTypes = {
+  value: React.PropTypes.object,
+  errors: React.PropTypes.object,
+  onChange: React.PropTypes.func
+};
+
+BoundFormRow.propTypes = {
+  name: React.PropTypes.string.isRequired
+};
+
+export default BoundFormRow;

src/components/FormRow.js

@@ -17,6 +17,12 @@ function determineElement(type) {
     type;
 }
 
+function parseFeedback(feedback) {
+  return typeof feedback === 'object' ?
+    [ null, { error: feedback } ] :
+    [ feedback, {} ];
+}
+
 const FormRow = props => {
   const {
     id,
@@ -35,8 +41,11 @@ const FormRow = props => {
 
   const InputElement = determineElement(type);
 
+  const [ baseFeedback, childFeedback ]= parseFeedback(feedback);
+  const rowColor = color || state || (baseFeedback && 'danger');
+
   return (
-    <FormGroup row color={color || state}>
+    <FormGroup row color={rowColor}>
       <Label for={id} sm={3} size={size}>
         {label}
         {required && label ? <span className="text-danger"> *</span> : null}
@@ -45,13 +54,14 @@ const FormRow = props => {
         <InputElement
           id={id}
           size={size}
-          state={color || state}
+          state={rowColor}
           type={type}
           children={React.Children.map(children, child => React.cloneElement(child, { type }))}
           {...attributes}
+          {...childFeedback}
         />
         {hint ? <FormText color="muted" children={hint} /> : null}
-        {feedback ? <FormFeedback children={feedback} /> : null}
+        {baseFeedback ? <FormFeedback children={baseFeedback} /> : null}
       </Col>
     </FormGroup>
   );
@@ -60,7 +70,10 @@ const FormRow = props => {
 FormRow.propTypes = {
   label: React.PropTypes.string,
   hint: React.PropTypes.string,
-  feedback: React.PropTypes.string,
+  feedback: React.PropTypes.oneOfType([
+    React.PropTypes.string,
+    React.PropTypes.object
+  ]),
   required: React.PropTypes.bool,
   type: React.PropTypes.oneOfType([
     React.PropTypes.string,

src/index.js

@@ -69,6 +69,7 @@ import AddressInput from './components/AddressInput.js';
 import Alert from './components/Alert.js';
 import BlockPanel from './components/BlockPanel.js';
 import BoundForm from './components/BoundForm';
+import BoundFormRow from './components/BoundFormRow';
 import CurrencyInput from './components/CurrencyInput.js';
 import Datapair from './components/Datapair.js';
 import DateMonth from './components/datemonth/DateMonth.js';
@@ -160,6 +161,7 @@ export {
   Alert,
   BlockPanel,
   BoundForm,
+  BoundFormRow,
   CurrencyInput,
   Datapair,
   DateMonth,

stories/Forms.js

@@ -2,7 +2,7 @@ import React from 'react';
 import { Container, Input } from 'reactstrap';
 import { storiesOf, action } from '@kadira/storybook';
 
-import { BoundForm, FormRow, FormChoice, CurrencyInput, AddressInput } from '../src';
+import { BoundForm, BoundFormRow, FormRow, FormChoice, CurrencyInput, AddressInput } from '../src';
 import { text, boolean, number, object, select } from '@kadira/storybook-addon-knobs';
 
 const formData = {
@@ -84,27 +84,27 @@ storiesOf('Forms', module)
       object={formData}
       errors={object('errors', { lastName: "can't be blank" })}
       onSubmit={action('submit')}>
-      <FormRow label="First Name" name="firstName" />
-      <FormRow label="Last Name" name="lastName" required />
-      <FormRow type={CurrencyInput} label="How much would you pay to meet the cast?" name="amount" />
-      <FormRow type="select" label="Select Movie" name="movie">
+      <BoundFormRow label="First Name" name="firstName" />
+      <BoundFormRow label="Last Name" name="lastName" required />
+      <BoundFormRow type={CurrencyInput} label="How much would you pay to meet the cast?" name="amount" />
+      <BoundFormRow type="select" label="Select Movie" name="movie">
         <FormChoice>A New Hope</FormChoice>
         <FormChoice value="episode6">The Empire Strikes Back</FormChoice>
         <FormChoice>The Force Awakens</FormChoice>
-      </FormRow>
-      <FormRow type="checkbox" label="Select the character(s) you like" name="characters">
+      </BoundFormRow>
+      <BoundFormRow type="checkbox" label="Select the character(s) you like" name="characters">
         <FormChoice>Darth Vader</FormChoice>
         <FormChoice>Luke Skywalker</FormChoice>
         <FormChoice disabled>Emperor Palpatine</FormChoice>
         <FormChoice value="awesome">Rey</FormChoice>
         <FormChoice>TK-421</FormChoice>
-      </FormRow>
-      <FormRow type="radio" label="Select Ship" name="ship">
+      </BoundFormRow>
+      <BoundFormRow type="radio" label="Select Ship" name="ship">
         <FormChoice>Death Star</FormChoice>
         <FormChoice>Millennium Falcon</FormChoice>
         <FormChoice value="shuttle">Imperial Shuttle</FormChoice>
-      </FormRow>
-      <FormRow type={AddressInput} name="address" label="Address" />
+      </BoundFormRow>
+      <BoundFormRow type={AddressInput} name="address" label="Address" />
       <button className="btn btn-primary">Submit</button>
     </BoundForm>
   ));

test/components/BoundForm.spec.js

@@ -7,7 +7,7 @@ import { shallow } from 'enzyme';
 import { Input } from 'reactstrap';
 
 import BoundForm from '../../src/components/BoundForm';
-import FormRow from '../../src/components/FormRow';
+import BoundFormRow from '../../src/components/BoundFormRow';
 import FormChoice from '../../src/components/FormChoice';
 
 describe('<BoundForm />', () => {
@@ -19,98 +19,66 @@ describe('<BoundForm />', () => {
     }
   };
 
-  data[undefined] = 'not good' // For testing items that don't have a name
-
   const errors = {
     firstName: "Can't be Glenn"
   }
 
   const submitFunc = sinon.stub();
   const changeFunc = sinon.stub();
 
-  const Composite = props => (
-    <div>
-      <Input name="address1" value={props.address1} />
-      <Input name="city" value={props.city} />
-    </div>
-  );
-
   const component = shallow(
-    <BoundForm object={data} errors={errors} onSubmit={submitFunc} onChange={changeFunc}>
-      <h1>Title</h1>
-      <FormRow label="First Name" name="firstName" />
-      <FormRow label="Last Name" name="lastName" />
-      <FormRow type="checkbox" label="Foobar" name="checkboxes">
-        <FormChoice name="vader">Darth Vader</FormChoice>
-      </FormRow>
-      <FormRow type={Composite} name="address" />
-      <FormRow type={Composite} name="thing" />
-    </BoundForm>
+    <BoundForm object={data} errors={errors} onSubmit={submitFunc} onChange={changeFunc} />
   );
 
-  it('should not add props to non-FormRows', () => {
-    const title = component.find('h1');
-    assert.equal(title.prop('value'), undefined);
-  });
-
-  it('should provide value from data object', () => {
-    const row = component.find('[name="firstName"]');
-    assert.equal(row.prop('value'), 'Glenn');
-  });
-
-  it('should provide default value for inputs without data', () => {
-    const row = component.find('[name="lastName"]');
-    assert.equal(row.prop('value'), '');
+  it('should provide a context', () => {
+    const context = component.instance().getChildContext();
+    assert.equal(context.value, component.state().formData);
+    assert.equal(context.errors, errors);
+    assert.equal(context.onChange, component.instance().handleChange);
   });
 
   it('should update data on change', () => {
-    const row = component.find('[name="firstName"]');
-    row.simulate('change', 'Desmond');
+    const firstNameOnChange = component.instance().handleChange('firstName');
+    firstNameOnChange('Desmond');
     assert.equal(data.firstName, 'Glenn');
     assert.equal(component.state('formData').firstName, 'Desmond');
   });
 
   it('should emit onChange when changed', () => {
-    const row = component.find('[name="firstName"]');
-    row.simulate('change', 'Desmond');
+    const firstNameOnChange = component.instance().handleChange('firstName');
+    firstNameOnChange('Desmond');
     assert(changeFunc.calledWith(Object.assign({}, data, { firstName: 'Desmond' })));
   });
 
-  it('should support nested data', () => {
-    const row = component.find('[name="address"]');
-    assert.equal(row.prop('value'), component.state('formData').address);
-  });
-
   it('should support updating nested data', () => {
-    const row = component.find('[name="address"]');
-    row.simulate('change', { address1: '456 cool' });
+    const addressOnChange = component.instance().handleChange('address');
+    addressOnChange({ address1: '456 cool' });
+
     assert.equal(data.address.address1, '123 awesome');
     assert.deepEqual(component.state('formData').address, { address1: '456 cool' });
   });
 
   it('should create nested data', () => {
-    const row = component.find('[name="thing"]');
-    row.simulate('change', { stuff: 'here' });
+    const thingOnChange = component.instance().handleChange('thing');
+    thingOnChange({ stuff: 'here' });
     assert.equal(data.thing, undefined);
     assert.deepEqual(component.state('formData').thing, { stuff: 'here' });
   });
 
+  it('should update data with target', () => {
+    const lastNameOnChange = component.instance().handleChange('lastName');
+    const target = document.createElement('input');
+    target.setAttribute('value', 'LastName');
+    lastNameOnChange({ target });
+
+    assert.equal(data.lastName, undefined);
+    assert.equal(component.state('formData').lastName, 'LastName');
+  });
+
   it('should submit with formData', () => {
     const preventDefault = sinon.stub();
     component.simulate('submit', { preventDefault });
     assert.equal(preventDefault.calledOnce, true);
     assert.equal(submitFunc.calledWith({ preventDefault }, component.state('formData')), true);
   });
-
-  it('should append errors', () => {
-    const row = component.find('[name="firstName"]');
-    assert.equal(row.prop('feedback'), errors.firstName);
-    assert.equal(row.prop('color'), 'danger');
-  });
-
-  it('should not have color for valid rows', () => {
-    const row = component.find('[name="address"]');
-    assert.equal(row.prop('feedback'), '');
-    assert.equal(row.prop('color'), null);
-  });
 });

test/components/BoundFormRow.spec.js

@@ -0,0 +1,58 @@
+import React from 'react';
+import sinon from 'sinon';
+import assert from 'assert';
+import { shallow } from 'enzyme';
+
+import BoundFormRow from '../../src/components/BoundFormRow';
+
+describe('<BoundFormRow />', () => {
+  describe('with context', () => {
+    const onChange = sinon.stub();
+    const component = shallow(
+      <BoundFormRow name="firstName" />, {
+        context: {
+          value: {
+            firstName: 'First Name',
+            lastName: 'Last Name'
+          },
+          errors: {
+            firstName: 'Not Right',
+            lastName: 'Something Else'
+          },
+          onChange
+        }
+      }
+    );
+
+    it('should forward props', () => {
+      assert.equal(component.prop('name'), 'firstName');
+    });
+
+    it('should pass the value from context', () => {
+      assert.equal(component.prop('value'), 'First Name');
+    });
+
+    it('should pass the error from context', () => {
+      assert.equal(component.prop('feedback'), 'Not Right');
+    });
+
+    it('should pass the onChange handler from context', () => {
+      assert.equal(component.prop('onChange'), onChange('firstName'));
+    });
+  });
+
+  describe('with defaults', () => {
+    const onChange = sinon.stub();
+    const component = shallow(
+      <BoundFormRow name="firstName" />, { context: { onChange } }
+    );
+
+    it('should set the default context value', () => {
+      assert.equal(component.prop('value'), '');
+    });
+
+    it('should set the default context errors', () => {
+      assert.equal(component.prop('feedback'), '');
+    });
+  });
+});