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.

Sign up for GitHub

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

[Select] Fix classes merge issue #11904

Merged
merged 2 commits into from
Jun 19, 2018
Merged

[Select] Fix classes merge issue #11904

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9693d9f
Add children and classes to input props for Select component
C-Rodg Jun 18, 2018
884068a
[Select] Fix classes merge issue
oliviertassinari Jun 19, 2018
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
18 changes: 11 additions & 7 deletions packages/material-ui/src/Select/Select.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import React from 'react';
import PropTypes from 'prop-types';
import SelectInput from './SelectInput';
import withStyles from '../styles/withStyles';
import mergeClasses from '../styles/mergeClasses';
import ArrowDropDownIcon from '../internal/svg-icons/ArrowDropDown';
import Input from '../Input';
import { styles as nativeSelectStyles } from '../NativeSelect/NativeSelect';
Expand Down Expand Up @@ -32,19 +33,15 @@ function Select(props) {
} = props;

const inputComponent = native ? NativeSelectInput : SelectInput;
const inputNativeProps = {
children,
classes,
IconComponent,
type: undefined, // We render a select. We can ignore the type provided by the `Input`.
};

return React.cloneElement(input, {
// Most of the logic is implemented in `SelectInput`.
// The `Select` component is a simple API wrapper to expose something better to play with.
inputComponent,
inputProps: {
...inputNativeProps,
children,
IconComponent,
type: undefined, // We render a select. We can ignore the type provided by the `Input`.
...(native
? {}
: {
Expand All @@ -59,6 +56,13 @@ function Select(props) {
SelectDisplayProps,
}),
...inputProps,
classes: inputProps
? mergeClasses({
baseClasses: classes,
newClasses: inputProps.classes,
Component: Select,
})
: classes,
...(input ? input.props.inputProps : {}),
},
...other,
Expand Down
27 changes: 22 additions & 5 deletions packages/material-ui/src/Select/Select.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@ describe('<Select />', () => {
let shallow;
let classes;
let mount;
const props = {
const defaultProps = {
input: <Input />,
children: [<MenuItem value="1">1</MenuItem>, <MenuItem value="2">2</MenuItem>],
};

before(() => {
shallow = createShallow({ dive: true });
classes = getClasses(<Select {...props} />);
classes = getClasses(<Select {...defaultProps} />);
mount = createMount();
});

Expand All @@ -25,18 +25,35 @@ describe('<Select />', () => {
});

it('should render a correct top element', () => {
const wrapper = shallow(<Select {...props} />);
const wrapper = shallow(<Select {...defaultProps} />);
assert.strictEqual(wrapper.type(), Input);
});

it('should provide the classes to the input component', () => {
const wrapper = shallow(<Select {...props} />);
const wrapper = shallow(<Select {...defaultProps} />);
assert.deepEqual(wrapper.props().inputProps.classes, classes);
});

describe('prop: inputProps', () => {
it('should be able to provide a custom classes property', () => {
const wrapper = shallow(
<Select
{...defaultProps}
inputProps={{
classes: { root: 'root' },
}}
/>,
);
assert.deepEqual(wrapper.props().inputProps.classes, {
...classes,
root: `${classes.root} root`,
});
});
});

it('should be able to mount the component', () => {
const wrapper = mount(
<Select {...props} value={10}>
<Select {...defaultProps} value={10}>
<MenuItem value="">
<em>None</em>
</MenuItem>
Expand Down
41 changes: 41 additions & 0 deletions packages/material-ui/src/styles/mergeClasses.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import warning from 'warning';
import getDisplayName from 'recompose/getDisplayName';

function mergeClasses(options = {}) {
const { baseClasses, newClasses, Component, noBase = false } = options;

if (!newClasses) {
return baseClasses;
}

return {
...baseClasses,
...Object.keys(newClasses).reduce((accumulator, key) => {
warning(
baseClasses[key] || noBase,
[
`Material-UI: the key \`${key}\` ` +
`provided to the classes property is not implemented in ${getDisplayName(Component)}.`,
`You can only override one of the following: ${Object.keys(baseClasses).join(',')}`,
].join('\n'),
);

warning(
!newClasses[key] || typeof newClasses[key] === 'string',
[
`Material-UI: the key \`${key}\` ` +
`provided to the classes property is not valid for ${getDisplayName(Component)}.`,
`You need to provide a non empty string instead of: ${newClasses[key]}.`,
Copy link

Choose a reason for hiding this comment

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

@oliviertassinari Sorry to bother you here but I think this may be an issue...
I see the condition || typeof newClasses[key] === 'string' on line 24 and then this message: ...You need to provide a non empty string instead of...
I'm confused... is this right? I'm getting this warning but it works (the class is applied... newClasses[key] is a non empty string).

root provided to the classes property is not valid for ListItem.
You need to provide a non empty string instead of: css-month-items-i14xvf

That's the message I get

Should this || be a &&? Or should the message be slightly different? Or... I'm completely wrong, of course...

Copy link
Member

Choose a reason for hiding this comment

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

@nicosommi Do you have a reproduction example?

Copy link

Choose a reason for hiding this comment

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

@oliviertassinari I found what is happening. As I suspected, it is not an issue with MUI :)
I was passing what the glamor css function returns (implementing my own override instead of using withStyles HOC), which is not a string but an object with a toString method on it. So it goes wrong through this validation, but when converted to string, it works. I solved by using toString() when passing it to MUI. So now no warning, and it works!
Sorry for the delayed response, I was digging into it and I just found the real cause. It may help for future reference if someone is trying to take a similar approach.

Copy link
Member

Choose a reason for hiding this comment

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

It's good to now. I guess it's better to be explicit about it. I have seen too many people try crazy stuff with this API.

Copy link

Choose a reason for hiding this comment

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

👍
In my case I'm trying to avoid using the HOC mainly because it is not friendly with the new React Context API (correct me if I'm wrong), and thus I created a standard function that process the style objects when I call it and pass the theme with a custom provider I created using this new context API with the render prop.

Copy link
Member

Choose a reason for hiding this comment

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

it is not friendly with the new React Context API

What does it mean?

Copy link

@nicosommi nicosommi Aug 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

What does it mean?

That it is using this instead of this

In practical terms the new context is used in a declarative way by rendering a consumer and using the children on the default render prop.

So my component wrapper looks like this:

      <ThemeProvider.Consumer>
        {theme => (
            <List classes={processStyleObjects(styles, theme)}>
              {this.props.children}
            </List>
         )}
      </ThemeProvider.Consumer>

Instead of creating the HOC based on the legacy context API.

Copy link
Member

Choose a reason for hiding this comment

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

It's an implementation detail. It doesn't matter to end users. You can very well using recompose toRenderProps helper with withTheme HOC. Or use the 11 lines render prop wrapper we document for withStyles.

Copy link

Choose a reason for hiding this comment

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

Indeed, it is.

You can very well using recompose toRenderProps helper with withTheme HOC

😲 Thank you for the tip! I didn't know about that helper

Or use the 11 lines render prop wrapper we document for withStyles

Do you have a link? I can't find that example (I'm reading here)

Copy link
Member

Choose a reason for hiding this comment

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

].join('\n'),
);

if (newClasses[key]) {
accumulator[key] = `${baseClasses[key]} ${newClasses[key]}`;
}

return accumulator;
}, {}),
};
}

export default mergeClasses;
45 changes: 7 additions & 38 deletions packages/material-ui/src/styles/withStyles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import contextTypes from 'react-jss/lib/contextTypes';
import { create } from 'jss';
import * as ns from 'react-jss/lib/ns';
import jssPreset from './jssPreset';
import mergeClasses from './mergeClasses';
import createMuiTheme from './createMuiTheme';
import themeListener from './themeListener';
import createGenerateClassName from './createGenerateClassName';
Expand Down Expand Up @@ -165,44 +166,12 @@ const withStyles = (stylesOrCreator, options = {}) => Component => {
}

if (generate) {
if (this.props.classes) {
this.cacheClasses.value = {
...this.cacheClasses.lastJSS,
...Object.keys(this.props.classes).reduce((accumulator, key) => {
warning(
this.cacheClasses.lastJSS[key] || this.disableStylesGeneration,
[
`Material-UI: the key \`${key}\` ` +
`provided to the classes property is not implemented in ${getDisplayName(
Component,
)}.`,
`You can only override one of the following: ${Object.keys(
this.cacheClasses.lastJSS,
).join(',')}`,
].join('\n'),
);

warning(
!this.props.classes[key] || typeof this.props.classes[key] === 'string',
[
`Material-UI: the key \`${key}\` ` +
`provided to the classes property is not valid for ${getDisplayName(
Component,
)}.`,
`You need to provide a non empty string instead of: ${this.props.classes[key]}.`,
].join('\n'),
);

if (this.props.classes[key]) {
accumulator[key] = `${this.cacheClasses.lastJSS[key]} ${this.props.classes[key]}`;
}

return accumulator;
}, {}),
};
} else {
this.cacheClasses.value = this.cacheClasses.lastJSS;
}
this.cacheClasses.value = mergeClasses({
baseClasses: this.cacheClasses.lastJSS,
newClasses: this.props.classes,
Component,
noBase: this.disableStylesGeneration,
});
}

return this.cacheClasses.value;
Expand Down