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] Improve styling solution #7461

Merged
merged 1 commit into from
Jul 22, 2017
Merged

[core] Improve styling solution #7461

merged 1 commit into from
Jul 22, 2017

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Jul 18, 2017

We should be pretty close to beta once this PR is merged.

Work In Progress

  • Fix multiple theme handling in withStyles
  • theme.md Show MuiThemeProvider nesting
  • usage.md without MuiThemeProvider
  • theme.md Show MuiThemeProvider in the introduction
  • insertionPoint as material-ui
  • css-in-js.md Update API
  • css-in-js.md Show how to use JSS provider of react-jss
  • server-side.md Update examples
  • testing.md Update API
  • createStyleSheet optional name
  • write new tests

Issues

Closes #7238 as a merge issue
Closes #6106 as using a broadcast approach
Closes #6129 as supporting theme nesting
Closes #6130 as I don't think it's needed in the core, we can write HOC for that
Closes #6821 as having interoperability with react-jss and
JssProvider
Closes #7310 as making the name optional and use the displayName when the name is not provided.

Wins

Some features this upgrade is unlocking:

  • Better perfs
  • Shorter class names in production
  • Better readable class names in development
  • Optional MuiThemeProvider, hence, less onboarding friction
  • Simpler createStyleSheet API with an optional name
  • Theme nesting
  • Slower theme update but reliable
  • Interoperability with react-jss

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jul 21, 2017

It should be almost good. I need to write new tests and we should be able to merge it ☯️ .
That's such a massive PR! That's definitely my biggest PR in terms of complexity and effort in the repository.

import withStyles from '../styles/withStyles';
import '../Button'; // So we don't have any override priority issue.
Copy link
Member Author

Choose a reason for hiding this comment

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

@kof This is the only limitation I have seen so far with the index trick over the previous global zIndex.

Copy link
Member Author

@oliviertassinari oliviertassinari Jul 22, 2017

Choose a reason for hiding this comment

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

The regression was catch by Argos-CI 😍 .

@oliviertassinari oliviertassinari merged commit b28a0e6 into mui:v1-alpha Jul 22, 2017
@oliviertassinari oliviertassinari deleted the upgrade-jss branch July 22, 2017 16:21
@oliviertassinari
Copy link
Member Author

@kof @iamstarkov feel free to have a look at the new styling solution here. Hopefully, at some point, we will be able to rely on theming and react-jss. At least, we know have a great e2e test: the documentation of Material-UI.

@nathanmarks RIP of jss-theme-reactor.

I wouldn't have been able to finish this PR without you guys, thanks a lot for pushing css-in-jss forward!

@oliviertassinari oliviertassinari added this to the v1.0.0-beta milestone Jul 22, 2017
@stunaz
Copy link
Contributor

stunaz commented Jul 22, 2017

That's a really great job @oliviertassinari , now we just have to wait for the release to enjoy it!

@oliviertassinari
Copy link
Member Author

Will soon release it as the first beta version. Thanks. Hopefully, we won't have any issue. I believe those changes are game changer.

@kof
Copy link
Contributor

kof commented Aug 1, 2017

One problem I am looking at right now with the latest mui is that now we can't have different Jss instances used for mui and for react-jss. The reason one may need them is plugins setup which might be different for mui than for the user code.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Aug 1, 2017

@kof We can always implement our own JssProvider to allow such pattern, I'm not sure we should allow it, why not just asking for a minimum set of plugins that Material-UI relies on?

@kof
Copy link
Contributor

kof commented Aug 1, 2017

My use case is to have a plugin which is applied to my user code but NOT to mui code, because mui will break in this case.

@kof
Copy link
Contributor

kof commented Aug 1, 2017

It is safer in general for mui to use an own jss instance I think.

@oliviertassinari
Copy link
Member Author

because mui will break in this case.

We could do the comparaison with Babel, I have never seen a babel plugin breaking existing code, as far as I know. Maybe that's where the issue is?

It is safer in general for mui to use an own jss instance I think.

That's opening the door for inconsistency issues, can we still share the same sheetRegistry for SSR?
We can go into that direction too.

@kof
Copy link
Contributor

kof commented Aug 1, 2017

That's opening the door for inconsistency issues,

you are right, if our road is towards using react-jss originally from both of them it would be expected both of them behave the same.

I am thinking right now that we should be able to disable/enable specific plugins where needed on specific sheets.

So for e.g. if I don't want mui to use jss-isolate plugin, I pass option createStyleSheet(styles, {isolate: false}) just for mui sheets. That option is already supported btw.

@oliviertassinari
Copy link
Member Author

@kof Yes, we can add this extra isolate: false change, that should be a one line change fix :).

@kof
Copy link
Contributor

kof commented Aug 1, 2017

Its just about passing any options user defines down to the style sheet I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants