[Proposal] Move styles to a single prop per component

[mbrookes]

Continuing the discussion here, this is a proposal to move styles to a single prop.

The main benefits are a significantly reduce API, and easy of expansion of styles without having to add additional props in the future.

@oliviertassinari:

I see two drawbacks:
people expect to use style property to work on our component as on a regular

.

Well, that's nice. But it doesn't. :)

We have multiple props to apply style, that work in unexpected ways (or not at all!). We could debate styles for the name of combined style prop, but at least keeping style is consistent is consistent with the current implementation, i.e. one less change, but I'm agnostic.

I feel like it will make the shouldComponentUpdate implementation harder

Could you expand on that? It doesn't seem to affect current implementation, but maybe that's a problem in its own right (no shouldComponentUpdate). And watching this for change in a future implementation is no harder this way than multiple props, unless I'm misunderstanding the issue.

Rough and ready discussion-draft PR incoming.

[oliviertassinari]

Actually, this situation reminds me one of a talk I have seens. The idea was. If at some point, you feel the API surface is too big. Break the component down into smaller bits. Well, that's what @newoga proposed in his issue. Do you think that It would address this ?

[mbrookes]

I think I know the issue you're referring to, (and sorry @newoga's for not coming back to comment on it), but in a nutshell I felt it was a step too far, breaking a single functional component up into too many component parts that aren't naturally shareable in the way say Divider is.

This approach retains the status-quo for component count (but doesn't prevent them being further modularised where appropriate), and is a logical extension of the current API.

Please take a look at the PR. It only tackles the top level component, but already the complexExample code is simpler, and the component code is no more complex than it was. The reduction in props is the main benefit.

You could argue that it's moving the problem from one place to another - multiple props transfer to multiple object keys, but it's a logical grouping of style related props under style (or styles), whereas props is an increasingly long list with multiple concerns.

[newoga]

I love simplifying the props, I really do. And I think this is something we should still fix. But personally I'm not a huge fan of shoving all styles in a single object as a fix 😄

My comment here is related to some of this discussion: #3043 (comment)

I feel like it will make the shouldComponentUpdate implementation harder

I agree with this. The more nesting involved, the constrained and difficult it is to determine changes since we have to do more expensive deep comparisons.

I think this whole topic is very much related to investigating new inline styles frameworks, how to make theming components easier through HOC, and we shouldn't consider them separately. For example even though this simplifies the props, it doesn't solve some of the other problems we've been having in terms of how to easily hook into component styling based on state (such as error and focus among other things).

I believe there is a nice functional approach to solving this problem. I have some ideas but it's not worth investigating until we clean up the code a little bit more. (I think we're getting closer though).

[newoga]

(and sorry @newoga's for not coming back to comment on it),

No worries!

but in a nutshell I felt it was a step too far, breaking a single functional component up into too many component parts that aren't naturally shareable in the way say Divider is.

My defense to this is that we should encourage to developers to wrap material-ui components to make their own rather than trying to make a perfect component that fits everyone perfectly. To do this successfully, I think we need to give developers a bit of flexibility without complicating the API too much. I think the best way to do this is to separate the components (where it makes sense).

In my comment, I mentioned an approach that allows developers to build their own <CustomTextField /> component that looks and behaves exactly how they like, and then it's right back down "one level" deep.

[mbrookes]

I feel like it will make the shouldComponentUpdate implementation harder

I agree with this. The more nesting involved, the constrained and difficult it is to determine changes since we have to do more expensive deep comparisons.

Mm, okay. Now I see what @oliviertassinari was referring to. But isn't that handled by the fact that the object is destructured and passed to the subcomponents as thier top-level style as it was before? Won't the shouldComponentUpdate check happen there for those? (Sorry for the naive questions.).

I liked the result from a purely cosmetic standpoint, even if the technical solution is lacking!

My defense to this is that we should encourage to developers to wrap material-ui components to make their own rather than trying to make a perfect component that fits everyone perfectly.

I don't disagree, and we're moving that way with the stateless component theme, but I think we have to consider usability. Braking a component down into multiple reusable subcomponents is fine, but the majority should remain an internal implementation detail (or optional secondary construction), rather than users being required to implement a single simple component such as TextField from multiple nested component parts.

Bottom line though - I've understood from two authorities on the subject that this isn't the best approach, so experiment concluded! 😄

[newoga]

I liked the result from a purely cosmetic standpoint, even if the technical solution is lacking!

I agree and I like and would prefer your proposal more than what we currently have. But since I'm not sure if it addresses all the problems we're currently facing, I'd rather explore more approaches so we can avoid two breaking API changes and hopefully settle for one 😄