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

[core] Add a new classes property to all the components #6911

Merged
merged 1 commit into from May 22, 2017
Merged

[core] Add a new classes property to all the components #6911

merged 1 commit into from May 22, 2017

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented May 20, 2017

Why

Short term goal:

  • Expose the CSS as a public API. Similarly to how you can customise all the CSS of Material-UI per classes, you can now do the same per instance, unlocking maximum flexibility

Long term goal:

  • Unlock the possibility to propagate theme update thanks to an event system outside of React that has his limitation
  • Pave the way toward a naked Material-UI

What

@oliviertassinari oliviertassinari changed the title [core] Continue with the refactorization [core] Add a new classes property to all the components May 20, 2017
@oliviertassinari oliviertassinari requested review from nathanmarks, mbrookes and rosskevin and removed request for nathanmarks May 20, 2017 23:44
@muibot muibot added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI PR: Needs Review and removed PR: Needs Review labels May 20, 2017
@oliviertassinari oliviertassinari added breaking change and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels May 21, 2017
@oliviertassinari
Copy link
Member Author

That's probably the biggest refactorization I have ever done on Material-UI. After hours of tracking down all the issues, it's green 🚀 !

Copy link
Member

@rosskevin rosskevin left a comment

Choose a reason for hiding this comment

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

Great work! Thank you for all this effort. Only two trivial flow changes from my perspective. I'm in favor of merging this ASAP.


type Props = {
/**
* Dialog children, usually the included sub-components.
*/
children?: Element<*>,
/**
* Useful to extend the style applied to components.
*/
classes: any,
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be classes: Object, because it won't be a Function or scalar type.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have tried object with no luck. thanks for the tip!


const dialogProps = {
fullScreen,
function ResponsiveFullScreen(props: { width: string }) {
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 this style change. No objection, just curious why.

Copy link
Member Author

Choose a reason for hiding this comment

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

No real motivation, I was in that mood:

  1. Using named function over anonymous one everywhere possible (we have very few, if not zero components written with arrow-function), so reducing entropy.
  2. Removing intermediary variables

src/Grid/Grid.js Outdated

type Props = {
/**
* The content of the component.
*/
children?: Element<*>,
/**
* Useful to extend the style applied to components.
*/
classes: any,
Copy link
Member

Choose a reason for hiding this comment

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

classes: Object

@oliviertassinari oliviertassinari merged commit cc591b1 into mui:next May 22, 2017
@oliviertassinari oliviertassinari deleted the hoc-migration branch May 22, 2017 17:12
@muibot muibot added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels May 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants