[Select] Fix classes merge issue

[C-Rodg]

Hey,
I have added children and classes to the inputProps for the Select component, which prevents the removal of default styling mentioned in #11867.

Thanks!

Closes #11867

[oliviertassinari]

@C-Rodg I don't understand, this logic was removed in #11873. It's repetitive logic.

Regarding solving #11867. I think that we should be extracting the classes merge utils out from the withStyles.js module and use it in the Select.js module. We would also need to add a unit test to make sure the logic is correct.

[C-Rodg]

@oliviertassinari Ahhh, sorry, I must've misunderstood the issue. I'm not very familiar with the withStyles module or styling MUI in-general so I may not be the best person to take this on, but if you want to provide a little more guidance I'd be more than happy to make an attempt at least.

[oliviertassinari]

@C-Rodg This is the merge classes logic I was referring to and that we could put into a module and use:
https://github.com/mui-org/material-ui/blob/ad4df1c16a9070582250db941dd97854215bc31f/packages/material-ui/src/styles/withStyles.js#L168-L202

[oliviertassinari]

@C-Rodg I'm continuing this pull-request.

[nicosommi]

@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...

[oliviertassinari]

@nicosommi Do you have a reproduction example?

[nicosommi]

@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.

[oliviertassinari]

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.

[nicosommi]

👍
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.

[oliviertassinari]

it is not friendly with the new React Context API

What does it mean?

[nicosommi]

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.

[oliviertassinari]

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.

[nicosommi]

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)

[oliviertassinari]

• https://github.com/acdlite/recompose/blob/master/src/packages/recompose/toRenderProps.js
• https://material-ui.com/customization/css-in-js/#render-props-api-11-lines-