Refactor switches, simplify code, fix related bugs.

- Refactors SwitchBase into a factory - Refactors Checkbox, Radio, Switch to use new Factory - Drastically simplify RadioGroup
mui · Dec 13, 2016 · a31f23d · a31f23d
1 parent 898d1d7
commit a31f23d
Show file tree

Hide file tree

Showing 24 changed files with 520 additions and 1,197 deletions.
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -28,6 +28,7 @@ module.exports = {
     'no-console': 'error', // airbnb is using warn
     'no-param-reassign': 'off',
     'no-prototype-builtins': 'off',
+    'no-use-before-define': ['error', { 'functions': false }], // airbnb have functions: true, annoying
     'object-curly-spacing': 'off', // use babel plugin rule
     'operator-linebreak': ['error', 'after'], // aibnb is disabling this rule
     'babel/object-curly-spacing': ['error', 'always'],

diff --git a/docs/site/src/demos/selection-controls/RadioButtonsGroup.js b/docs/site/src/demos/selection-controls/RadioButtonsGroup.js
@@ -32,6 +32,7 @@ export default class RadioButtonsGroup extends Component {
         <FormLabel required>Gender</FormLabel>
         <RadioGroup
           aria-label="Gender"
+          name="gender"
           className={classes.group}
           selectedValue={this.state.selectedValue}
           onChange={this.handleChange}

diff --git a/docs/site/src/demos/selection-controls/SwitchLabels.js b/docs/site/src/demos/selection-controls/SwitchLabels.js
@@ -16,7 +16,6 @@ export default class SwitchLabels extends Component {
           checked={this.state.checkedA}
           onChange={(event, checked) => this.setState({ checkedA: checked })}
           label="A"
-          labelReverse
         />
         <Switch
           checked={this.state.checkedB}

diff --git a/src/Checkbox/Checkbox.js b/src/Checkbox/Checkbox.js
@@ -1,10 +1,8 @@
 // @flow weak
 
-import React, { PropTypes } from 'react';
 import { createStyleSheet } from 'jss-theme-reactor';
-import classNames from 'classnames';
-import SwitchBase from '../internal/SwitchBase';
-import SelectionLabel from '../internal/SelectionLabel';
+import { createSwitch } from '../internal/SwitchBase';
+import { withSwitchLabel } from '../internal/SwitchLabel';
 
 export const styleSheet = createStyleSheet('Checkbox', (theme) => {
   return {
@@ -20,79 +18,10 @@ export const styleSheet = createStyleSheet('Checkbox', (theme) => {
   };
 });
 
-export default function Checkbox(props, context) {
-  const {
-    className,
-    checkedClassName,
-    disabled,
-    disabledClassName,
-    label,
-    labelClassName,
-    labelReverse,
-    ...other
-  } = props;
-  const classes = context.styleManager.render(styleSheet);
+const Checkbox = createSwitch({ styleSheet });
 
-  const switchProps = {
-    className: classNames(classes.default, className),
-    checkedClassName: classNames(classes.checked, checkedClassName),
-    disabled,
-    disabledClassName: classNames(classes.disabled, disabledClassName),
-    ...other,
-  };
-
-  if (label) {
-    return (
-      <SelectionLabel
-        label={label}
-        labelReverse={labelReverse}
-        className={labelClassName}
-        disabled={disabled}
-      >
-        <SwitchBase
-          aria-label={label}
-          {...switchProps}
-        />
-      </SelectionLabel>
-    );
-  }
-
-  return <SwitchBase {...switchProps} />;
-}
-
-Checkbox.propTypes = {
-  checkedClassName: PropTypes.string,
-  /**
-   * The CSS class name of the root element.
-   */
-  className: PropTypes.string,
-  /**
-   * If `true`, the control will be disabled.
-   */
-  disabled: PropTypes.bool,
-  /**
-   * The CSS class name of the switch element when disabled.
-   */
-  disabledClassName: PropTypes.string,
-  /**
-   * The text to be used in an enclosing label element.
-   */
-  label: PropTypes.node,
-  /**
-   * The className to be used in an enclosing label element.
-   */
-  labelClassName: PropTypes.string,
-  /**
-   * Will reverse the order of the element and the label.
-   */
-  labelReverse: PropTypes.bool,
-};
+Checkbox.displayName = 'Checkbox';
 
-Checkbox.defaultProps = {
-  disabled: false,
-  labelReverse: false,
-};
+export { Checkbox };
 
-Checkbox.contextTypes = {
-  styleManager: PropTypes.object.isRequired,
-};
+export default withSwitchLabel(Checkbox);
diff --git a/src/Checkbox/Checkbox.spec.js b/src/Checkbox/Checkbox.spec.js
@@ -1,10 +1,9 @@
 // @flow weak
 /* eslint-env mocha */
 
-import React from 'react';
 import { assert } from 'chai';
 import { createShallowWithContext } from 'test/utils';
-import Checkbox, { styleSheet } from './Checkbox';
+import LabelCheckbox, { Checkbox, styleSheet } from './Checkbox';
 
 describe('<Checkbox />', () => {
   let shallow;
@@ -15,68 +14,25 @@ describe('<Checkbox />', () => {
     classes = shallow.context.styleManager.render(styleSheet);
   });
 
-  it('should render a SwitchBase when label not present', () => {
-    const wrapper = shallow(
-      <Checkbox />,
-    );
-    assert.strictEqual(wrapper.is('SwitchBase'), true, 'should be a SwitchBase');
-  });
-
-  it('should render a label', () => {
-    const wrapper = shallow(
-      <Checkbox label="Foo" />,
-    );
-    assert.strictEqual(wrapper.is('SelectionLabel'), true, 'should be a SelectionLabel');
-  });
-
-  it('should render with the default and checked classes', () => {
-    const wrapper = shallow(
-      <Checkbox
-        checked
-        label="Foo"
-        labelClassName="foo"
-        className="woof"
-        checkedClassName="meow"
-      />,
-    );
-    const switchBase = wrapper.find('SwitchBase');
-    assert.strictEqual(wrapper.hasClass('foo'), true, 'should have the "foo" class');
-    assert.strictEqual(switchBase.hasClass('woof'), true, 'should have the "woof" class');
-    assert.strictEqual(switchBase.hasClass(classes.default), true, 'should have the default class');
-    assert.strictEqual(
-      switchBase.prop('checkedClassName').indexOf('meow') !== -1,
-      true,
-      'should have the "meow" class',
-    );
-    assert.strictEqual(
-      switchBase.prop('checkedClassName').indexOf(classes.checked) !== -1,
-      true,
-      'should have the checked class',
-    );
-  });
-
-  it('should spread custom props on the switchBase node', () => {
-    const wrapper = shallow(<Checkbox label="Foo" data-my-prop="woof" />);
-    const switchBase = wrapper.find('SwitchBase');
-    assert.strictEqual(switchBase.prop('data-my-prop'), 'woof', 'custom prop should be woof');
+  describe('styleSheet', () => {
+    it('should have the classes required for SwitchBase', () => {
+      assert.strictEqual(typeof classes.default, 'string');
+      assert.strictEqual(typeof classes.checked, 'string');
+      assert.strictEqual(typeof classes.disabled, 'string');
+    });
   });
 
-  describe('prop: disabled', () => {
-    it('should disable the component', () => {
-      const wrapper = shallow(<Checkbox disabled />);
-      assert.strictEqual(wrapper.props().disabled, true, 'should pass the property down the tree');
+  describe('named Checkbox export', () => {
+    it('should be a SwitchBase with the displayName set for debugging', () => {
+      assert.strictEqual(Checkbox.name, 'SwitchBase');
+      assert.strictEqual(Checkbox.displayName, 'Checkbox');
     });
   });
 
-  describe('prop: disabledClassName', () => {
-    it('should provide the class', () => {
-      const className = 'foo';
-      const wrapper = shallow(<Checkbox disabledClassName={className} />);
-      assert.strictEqual(
-        wrapper.find('SwitchBase').props().disabledClassName.indexOf(className) !== -1,
-        true,
-        'should have the custom disabled class',
-      );
+  describe('default export', () => {
+    it('should be Checkbox wrapped with SwitchLabel', () => {
+      assert.strictEqual(LabelCheckbox.name, 'SwitchLabel');
+      assert.strictEqual(LabelCheckbox.displayName, 'withSwitchLabel(Checkbox)');
     });
   });
 });
diff --git a/src/IconButton/IconButton.js b/src/IconButton/IconButton.js
@@ -62,6 +62,7 @@ export const styleSheet = createStyleSheet('IconButton', (theme) => {
 export default function IconButton(props, context) {
   const {
     accent,
+    buttonRef,
     children,
     className,
     contrast,
@@ -80,6 +81,7 @@ export default function IconButton(props, context) {
       centerRipple
       keyboardFocusedClassName={classes.keyboardFocused}
       disabled={disabled}
+      ref={buttonRef}
       {...other}
     >
       <span className={classNames(classes.label)}>
@@ -96,6 +98,10 @@ IconButton.propTypes = {
    * If true, will use the theme's accent color.
    */
   accent: PropTypes.bool,
+  /**
+   * @ignore
+   */
+  buttonRef: PropTypes.func,
   /**
    * The icon element. If a string is passed,
    * it will be used as a material icon font ligature.

diff --git a/src/Radio/Radio.js b/src/Radio/Radio.js
@@ -1,10 +1,8 @@
 // @flow weak
 
-import React, { PropTypes } from 'react';
 import { createStyleSheet } from 'jss-theme-reactor';
-import classNames from 'classnames';
-import SwitchBase from '../internal/SwitchBase';
-import SelectionLabel from '../internal/SelectionLabel';
+import { createSwitch } from '../internal/SwitchBase';
+import { withSwitchLabel } from '../internal/SwitchLabel';
 
 export const styleSheet = createStyleSheet('Radio', (theme) => {
   return {
@@ -20,90 +18,15 @@ export const styleSheet = createStyleSheet('Radio', (theme) => {
   };
 });
 
-export default function Radio(props, context) {
-  const {
-    className,
-    checkedClassName,
-    disabled,
-    disabledClassName,
-    label,
-    labelClassName,
-    labelReverse,
-    onChange,
-    value,
-    ...other
-  } = props;
-  const classes = context.styleManager.render(styleSheet);
-
-  const switchProps = {
-    className: classNames(classes.default, className),
-    checkedClassName: classNames(classes.checked, checkedClassName),
-    icon: 'radio_button_unchecked',
-    checkedIcon: 'radio_button_checked',
-    type: 'radio',
-    value,
-    onChange,
-    disabled,
-    disabledClassName: classNames(classes.disabled, disabledClassName),
-    ...other,
-  };
-
-  if (label) {
-    return (
-      <SelectionLabel
-        label={label}
-        labelReverse={labelReverse}
-        className={labelClassName}
-        disabled={disabled}
-      >
-        <SwitchBase
-          aria-label={label}
-          {...switchProps}
-        />
-      </SelectionLabel>
-    );
-  }
-
-  return <SwitchBase {...switchProps} />;
-}
-
+const Radio = createSwitch({
+  styleSheet,
+  inputType: 'radio',
+  defaultIcon: 'radio_button_unchecked',
+  defaultCheckedIcon: 'radio_button_checked',
+});
 
-Radio.propTypes = {
-  checkedClassName: PropTypes.string,
-  /**
-   * The CSS class name of the root element.
-   */
-  className: PropTypes.string,
-  /**
-   * If `true`, the control will be disabled.
-   */
-  disabled: PropTypes.bool,
-  /**
-   * The CSS class name of the switch element when disabled.
-   */
-  disabledClassName: PropTypes.string,
-  /**
-   * The text to be used in an enclosing label element.
-   */
-  label: PropTypes.node,
-  /**
-   * The className to be used in an enclosing label element.
-   */
-  labelClassName: PropTypes.string,
-  /**
-   * Will reverse the order of the element and the label.
-   */
-  labelReverse: PropTypes.bool,
-  name: PropTypes.string,
-  onChange: PropTypes.func,
-  value: PropTypes.string,
-};
+Radio.displayName = 'Radio';
 
-Radio.defaultProps = {
-  disabled: false,
-  labelReverse: false,
-};
+export { Radio };
 
-Radio.contextTypes = {
-  styleManager: PropTypes.object.isRequired,
-};
+export default withSwitchLabel(Radio);