Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor switches, simplify code, fix related bugs. #5780

Merged
merged 8 commits into from
Dec 15, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about the motivation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oliviertassinari I wanted to put all the test helpers below the main part of the suite in one case. Functions are hoisted so there isn't any harm and in some scenarios it helps you leave more important stuff at the top. It isn't something that affects our react components because for the most part we have non-hoistables (class, assignment).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a fight between important stuff at the top vs load the context before digging into the code. I prefer the second option but some flexibility won't harm 👍 .

Copy link
Member Author

@nathanmarks nathanmarks Dec 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I see that side too -- but on the other hand it can be confusing to open a file and it's hard to find the root describe() in a test because there's a bunch of helper functions first. In that case, you don't even know what they are for yet.

'object-curly-spacing': 'off', // use babel plugin rule
'operator-linebreak': ['error', 'after'], // aibnb is disabling this rule
'babel/object-curly-spacing': ['error', 'always'],
Expand Down
3 changes: 2 additions & 1 deletion docs/site/src/demos/dialogs/ConfirmationDialog.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
DialogActions,
DialogTitle,
} from 'material-ui/Dialog';
import { Radio, RadioGroup } from 'material-ui/Radio';
import { LabelRadio as Radio, RadioGroup } from 'material-ui/Radio';

const options = [
'None',
Expand Down Expand Up @@ -82,6 +82,7 @@ class ConfirmationDialog extends Component {
<RadioGroup
ref={(c) => { this.radioGroup = c; }}
aria-label="Gender"
name="gender"
selectedValue={this.state.selectedValue}
onChange={this.handleChange}
>
Expand Down
12 changes: 6 additions & 6 deletions docs/site/src/demos/selection-controls/Checkboxes.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// @flow weak

import React, { Component } from 'react';
import Checkbox from 'material-ui/Checkbox';
import { LabelCheckbox } from 'material-ui/Checkbox';
import { FormGroup } from 'material-ui/Form';

export default class Checkboxes extends Component {
Expand All @@ -13,28 +13,28 @@ export default class Checkboxes extends Component {
render() {
return (
<FormGroup row>
<Checkbox
<LabelCheckbox
checked={this.state.checkedA}
onChange={(event, checked) => this.setState({ checkedA: checked })}
label="Option A"
value="checkedA"
/>
<Checkbox
<LabelCheckbox
checked={this.state.checkedB}
onChange={(event, checked) => this.setState({ checkedB: checked })}
label="Option B"
value="checkedB"
/>
<Checkbox
<LabelCheckbox
label="Option C"
value="checkedC"
/>
<Checkbox
<LabelCheckbox
disabled
label="Disabled"
value="checkedD"
/>
<Checkbox
<LabelCheckbox
checked
disabled
label="Disabled"
Expand Down
11 changes: 6 additions & 5 deletions docs/site/src/demos/selection-controls/RadioButtonsGroup.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import React, { Component, PropTypes } from 'react';
import { createStyleSheet } from 'jss-theme-reactor';
import { Radio, RadioGroup } from 'material-ui/Radio';
import { LabelRadio, RadioGroup } from 'material-ui/Radio';
import { FormLabel } from 'material-ui/Form';

const styleSheet = createStyleSheet('RadioButtonsGroup', () => ({
Expand Down Expand Up @@ -32,14 +32,15 @@ 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}
>
<Radio label="Male" value="male" />
<Radio label="Female" value="female" />
<Radio label="Other" value="other" />
<Radio label="Disabled" value="disabled" disabled />
<LabelRadio label="Male" value="male" />
<LabelRadio label="Female" value="female" />
<LabelRadio label="Other" value="other" />
<LabelRadio label="Disabled" value="disabled" disabled />
</RadioGroup>
</div>
);
Expand Down
9 changes: 4 additions & 5 deletions docs/site/src/demos/selection-controls/SwitchLabels.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// @flow weak

import React, { Component } from 'react';
import Switch from 'material-ui/Switch';
import { LabelSwitch } from 'material-ui/Switch';

export default class SwitchLabels extends Component {
state = {
Expand All @@ -12,18 +12,17 @@ export default class SwitchLabels extends Component {
render() {
return (
<div>
<Switch
<LabelSwitch
checked={this.state.checkedA}
onChange={(event, checked) => this.setState({ checkedA: checked })}
label="A"
labelReverse
/>
<Switch
<LabelSwitch
checked={this.state.checkedB}
onChange={(event, checked) => this.setState({ checkedB: checked })}
label="B"
/>
<Switch label="C" disabled />
<LabelSwitch label="C" disabled />
</div>
);
}
Expand Down
83 changes: 7 additions & 76 deletions src/Checkbox/Checkbox.js
Original file line number Diff line number Diff line change
@@ -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/withSwitchLabel';

export const styleSheet = createStyleSheet('Checkbox', (theme) => {
return {
Expand All @@ -20,79 +18,12 @@ 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.displayName = 'Checkbox';

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,
};
export default Checkbox;

Checkbox.defaultProps = {
disabled: false,
labelReverse: false,
};
const LabelCheckbox = withSwitchLabel(Checkbox);

Checkbox.contextTypes = {
styleManager: PropTypes.object.isRequired,
};
export { LabelCheckbox };
74 changes: 15 additions & 59 deletions src/Checkbox/Checkbox.spec.js
Original file line number Diff line number Diff line change
@@ -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 Checkbox, { LabelCheckbox, styleSheet } from './Checkbox';

describe('<Checkbox />', () => {
let shallow;
Expand All @@ -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('default 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('named LabelCheckbox export', () => {
it('should be Checkbox wrapped with SwitchLabel', () => {
assert.strictEqual(LabelCheckbox.name, 'SwitchLabel');
assert.strictEqual(LabelCheckbox.displayName, 'withSwitchLabel(Checkbox)');
});
});
});
2 changes: 1 addition & 1 deletion src/Checkbox/index.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* eslint-disable flowtype/require-valid-file-annotation */

export default from './Checkbox';
export Checkbox from './Checkbox';
export Checkbox, { LabelCheckbox } from './Checkbox';
4 changes: 2 additions & 2 deletions src/Form/FormGroup.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,14 @@ export const styleSheet = createStyleSheet('FormGroup', () => {
* awareness. Upon focusing on one of the child controls, it will propagate `focused` to the label.
*/
export default function FormGroup(props, context) {
const { className, children, row } = props;
const { className, children, row, ...other } = props;
const classes = context.styleManager.render(styleSheet);
const rootClassName = classNames(classes.root, {
[classes.row]: row,
}, className);

return (
<div className={rootClassName}>
<div className={rootClassName} {...other}>
{children}
</div>
);
Expand Down
6 changes: 6 additions & 0 deletions src/IconButton/IconButton.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ export const styleSheet = createStyleSheet('IconButton', (theme) => {
export default function IconButton(props, context) {
const {
accent,
buttonRef,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

children,
className,
contrast,
Expand All @@ -80,6 +81,7 @@ export default function IconButton(props, context) {
centerRipple
keyboardFocusedClassName={classes.keyboardFocused}
disabled={disabled}
ref={buttonRef}
{...other}
>
<span className={classNames(classes.label)}>
Expand All @@ -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.
Expand Down
Loading