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

[POC] Styled render callback component and refactored withStyles #9503

Closed
wants to merge 21 commits into from
Closed
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
1362466
switch prettier config to yaml
rosskevin Dec 15, 2017
a36895b
prototype - Styled render callback and withStyles refactoring
rosskevin Dec 15, 2017
f045ce6
fix getClasses helper to mount and obtain the styleSheet via sheetsRe…
rosskevin Dec 15, 2017
73fbad0
bad prop
rosskevin Dec 15, 2017
bf74fdd
rename dummy field class name to prevent confusion
rosskevin Dec 15, 2017
a56befa
I’m wrong about the props being modified - this proves it.
rosskevin Dec 15, 2017
0fe9008
hack showing difference causing sheet growth
rosskevin Dec 15, 2017
8be0604
proof that getStylesCreator creates a different result with same styl…
rosskevin Dec 15, 2017
52e2603
oops, NOW this proves it. sneaky.
rosskevin Dec 15, 2017
bb911be
add broken styles function equality test
rosskevin Dec 15, 2017
f42d156
fixed creator as key problem
rosskevin Dec 15, 2017
c042d5c
move the indexCounter into getStylesCreator - more relevant there.
rosskevin Dec 15, 2017
caece9a
fix passing of classes prop
rosskevin Dec 15, 2017
926db46
reestablish innerRef on withStyles
rosskevin Dec 15, 2017
8ff4562
ensure different creators for different style objects - just being pa…
rosskevin Dec 15, 2017
e4934e5
tests checkpoint
rosskevin Dec 15, 2017
7abdf6d
fix resolution of listenToTheme
rosskevin Dec 15, 2017
7717eba
Add tests for withStyles (withTheme) and fix Styled
rosskevin Dec 16, 2017
7060920
more fixes - first other spec green Paper.spec - should be reviewed b…
rosskevin Dec 16, 2017
ff5a53d
name should not be propagated
rosskevin Dec 16, 2017
a41f9e8
decouple props and establish render callback signature
rosskevin Dec 16, 2017
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
Prev Previous commit
Next Next commit
fix passing of classes prop
rosskevin committed Dec 15, 2017
commit caece9aaa1e8372a055d2a561d12106568a79b4c
13 changes: 11 additions & 2 deletions src/styles/withStyles.js
Original file line number Diff line number Diff line change
@@ -1,18 +1,27 @@
import React from 'react';
import PropTypes from 'prop-types';
import hoistNonReactStatics from 'hoist-non-react-statics';
import wrapDisplayName from 'recompose/wrapDisplayName';
import Styled from './Styled';

// Wrap a component in `Styled` to provide classes.
const withStyles = (stylesOrCreator, options = {}) => Component => {
const Style = props => {
const { classes, ...componentProps } = props;
return (
<Styled styles={stylesOrCreator} options={options} Component={Component}>
{styledProps => <Component {...styledProps} {...props} />}
<Styled classes={classes} Component={Component} options={options} styles={stylesOrCreator}>
{styledProps => <Component {...styledProps} {...componentProps} />}
</Styled>
);
};
Copy link
Member Author

Choose a reason for hiding this comment

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

This is an example where the render callback pattern is better. withTheme and innerRef are HOC options not necessary using Styled. With a render callback pattern, the child chooses what it uses and what it ignores (e.g. theme), and the innerRef indirection is unnecessary because the parent can put the ref where ever it chooses. This code shows what it took to match the various current usage patterns of the HOC.


Style.propTypes = {
/**
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this?

Copy link
Member

Choose a reason for hiding this comment

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

I mean, the propTypes ?

Copy link
Member Author

@rosskevin rosskevin Dec 15, 2017

Choose a reason for hiding this comment

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

innerRef is exclusive to withStyles, classes though can be omitted. --Actually, I don't think we need any (including innerRef), just pass-through now.

* Useful to extend the style applied to components.
*/
classes: PropTypes.object,
};

if (process.env.NODE_ENV !== 'production') {
Style.displayName = wrapDisplayName(Component, 'withStyles');
}
24 changes: 10 additions & 14 deletions src/styles/withStyles.spec.js
Original file line number Diff line number Diff line change
@@ -7,12 +7,9 @@ import withStyles, { preset } from './withStyles';
import MuiThemeProvider from './MuiThemeProvider';
import createMuiTheme from './createMuiTheme';
import createGenerateClassName from './createGenerateClassName';
import { createShallow, createMount, getClasses } from '../test-utils';
import { createMount, getClasses } from '../test-utils';
import consoleErrorMock from '../../test/utils/consoleErrorMock';

const styles = { root: { display: 'flex' } };
export { styles };

// eslint-disable-next-line react/prefer-stateless-function
class Empty extends React.Component {
render() {
@@ -21,11 +18,9 @@ class Empty extends React.Component {
}

describe('withStyles', () => {
let shallow;
let mount;

before(() => {
shallow = createShallow();
mount = createMount();
});

@@ -38,12 +33,13 @@ describe('withStyles', () => {
let classes;

before(() => {
const styles = { root: { display: 'flex' } };
StyledComponent1 = withStyles(styles, { name: 'MuiEmptyField' })(Empty);
classes = getClasses(<StyledComponent1 />);
});

it('should provide a classes property', () => {
mount(<StyledComponent1 />);
mount(<StyledComponent1 />); // attempt to grow sheet count - shouldn't change classes
mount(<StyledComponent1 />);
mount(<StyledComponent1 />);
const wrapper = mount(<StyledComponent1 />).find('Empty');
@@ -60,19 +56,19 @@ describe('withStyles', () => {
});

it('should accept a classes property', () => {
const wrapper = shallow(<StyledComponent1 classes={{ root: 'h1' }} />);
const wrapper = mount(<StyledComponent1 classes={{ root: 'h1' }} />).find('Empty');
assert.deepEqual(wrapper.props().classes, { root: `${classes.root} h1` });
assert.strictEqual(consoleErrorMock.callCount(), 0);
});

it('should ignore undefined property', () => {
const wrapper = shallow(<StyledComponent1 classes={{ root: undefined }} />);
const wrapper = mount(<StyledComponent1 classes={{ root: undefined }} />).find('Empty');
assert.deepEqual(wrapper.props().classes, { root: `${classes.root}` });
assert.strictEqual(consoleErrorMock.callCount(), 0);
});

it('should warn if providing a unknown key', () => {
const wrapper = shallow(<StyledComponent1 classes={{ bar: 'foo' }} />);
const wrapper = mount(<StyledComponent1 classes={{ bar: 'foo' }} />).find('Empty');

assert.deepEqual(wrapper.props().classes, { root: classes.root, bar: 'undefined foo' });
assert.strictEqual(consoleErrorMock.callCount(), 1);
@@ -83,7 +79,7 @@ describe('withStyles', () => {
});

it('should warn if providing a non string', () => {
const wrapper = shallow(<StyledComponent1 classes={{ root: {} }} />);
const wrapper = mount(<StyledComponent1 classes={{ root: {} }} />).find('Empty');

assert.deepEqual(wrapper.props().classes, { root: `${classes.root} [object Object]` });
assert.strictEqual(consoleErrorMock.callCount(), 2);
@@ -94,7 +90,7 @@ describe('withStyles', () => {
});

it('should recycle the object between two render if possible', () => {
const wrapper = mount(<StyledComponent1 />);
const wrapper = mount(<StyledComponent1 />).find('Empty');
const classes1 = wrapper.find(Empty).props().classes;
wrapper.update();
const classes2 = wrapper.find(Empty).props().classes;
@@ -105,7 +101,7 @@ describe('withStyles', () => {
describe('prop: ref', () => {
it('should provide a ref on the inner component', () => {
const handleRef = spy();
mount(<StyledComponent1 ref={handleRef} />);
mount(<StyledComponent1 ref={handleRef} />).find('Empty');
assert.strictEqual(handleRef.callCount, 1);
});
});
@@ -213,7 +209,7 @@ describe('withStyles', () => {
it('should take the new stylesCreator into account', () => {
const styles1 = { root: { padding: 1 } };
const StyledComponent1 = withStyles(styles1, { name: 'MuiEmptyField' })(Empty);
const wrapper = shallow(<StyledComponent1 />);
const wrapper = mount(<StyledComponent1 />).find('Empty');

const styles2 = { root: { padding: 2 } };
const StyledComponent2 = withStyles(styles2, { name: 'MuiEmptyField' })(Empty);