The Wayback Machine - https://web.archive.org/web/20200614110709/https://github.com/mui-org/material-ui/issues/6115
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

Migrate to styled-components #6115

Open
kybarg opened this issue Feb 11, 2017 · 144 comments
Open

Migrate to styled-components #6115

kybarg opened this issue Feb 11, 2017 · 144 comments
Assignees
Milestone

Comments

@kybarg
Copy link
Contributor

@kybarg kybarg commented Feb 11, 2017

Can we switch to styled-components?

Outdated comparison

It has many advantages against JSS
Here comparison table, and next version is even going to avoid SSR styles re-render!

Features styled-components react-jss
No build requirements
Small and lightweight
Supports global CSS
Supports entirety of CSS
Colocated
Isolated
Doesn’t break inline styles
Easy to override
Theming
Server side rendering
No wrapper components
ReactNative support

Legend: = Yes, = No, 😕 = Kinda, refer to notes or parentheses

@oliviertassinari

This comment was marked as outdated.

@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Feb 11, 2017

@kybarg Actually, I'm not sure to fully understand what you are suggesting.
We are not using react-jss as your question suggest.

When you say we, are you talking about users or Material-UI?

@kof
Copy link
Member

@kof kof commented Feb 11, 2017

My points are:

  • styled-components is a much higher level library than the JSS core, you can for sure use it in your application layer.
  • The comparison table above is about react-jss, not JSS core and is a subjective opinion of Max. It is partially not true and partially just a look from some specific perspective which we don't see from the table.
  • We are working on dynamic rules rendering for more efficient dynamic styling, jss-theme-reactor does the job right now already, its just a matter of an optimization, which is probably not very relevant for MUI.
  • What MUI uses internally should be totally out of concern for the end user. Everything a user needs to do should be possible without even knowing what MUI is internally using or at least its more or less regular cssinjs syntax for theming.
  • We need to get use cases MUI doesn't support right now and solve them. I am always happy to help the integration process and available on gitter.
  • What is react-native support anyways? Isn't it just a subset of the web platform? If so it should just work, otherwise let me know what JSS needs to be able to do to support react-native, here is the issue.
@mxstbr
Copy link

@mxstbr mxstbr commented Feb 11, 2017

That table is indeed very subjective and based on my own experience. FWIW, styled-components works with any third party component library:

import { Button } from 'material-ui'

const MyButton = styled(Button)`
  // Only these styles will be overridden
  background: red;
`

This works as long as the components attach the className prop internally to some DOM node:

const MaterialUIButton = (props) => {
  /* ... */
  // As long as props.className is attached, the above usage will work
  return <button className={`bla ${props.className}`} />
}

What is react-native support anyways? Isn't it just a subset of the web platform?

Nope, it's not quite that easy 😉 Adding support to JSS shouldn't be hard though, as all you have to do is pass the style object through to StyleSheet.create(). This requires a bit more effort from the styled-components side to make CSS strings work.


I've been talking to @javivelasco for a while now and love where he's going with react-toolbox. His implementation is amazing, and I'd love to see all third party component libraries adopt it. Pinging him so he can chime in with his ideas here!


No server-side rendering concurrency possible. It's relying on a singleton to collect the styles while JSS instantiate a new instance to collect styles at each request. Steaming is really limited.

Totally unrelated, mind commenting in this issue with your ideas for an API that would allow this to be the case? We haven't decided what we're going to do, so your input would very much be appreciated.

@kybarg kybarg closed this Feb 17, 2017
@rainice
Copy link

@rainice rainice commented Mar 17, 2017

Hi, I inquired about this in gitter. Just to get the views of others, I will post it here as well:

I know material-ui next is heavily invested in a custom jss solution.
Does anyone discovered any serious advantage of using jss over styled-components?

While jss is good as it enables several patterns like decorators (injectstyles) and plugins, I think styled-components straightforward approach is much cleaner as there is no need for decorators, custom setup and plugins cause it doesn't need to.

In styled-comp every component is already styled so no need to pass styles. and you pass props that can evaluated to produce a different style
no setup (createJss)
no plugins (prefix)
no JSON DSL

@kof
Copy link
Member

@kof kof commented Mar 17, 2017

Somebody has to lock this thread.

@rainice jss doesn't has decorators, a HOC is an implementation detail of react-jss and is not used here.

@javivelasco

This comment was marked as outdated.

@kof

This comment was marked as outdated.

@mbrookes

This comment was marked as outdated.

@mbrookes mbrookes closed this Mar 17, 2017
@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Mar 17, 2017

I wish we could have moved forward that thread with a less coupled styling solution!
However, I think that our priority, for now, should be around finishing the migration/overall improvement of the components.

@oliviertassinari oliviertassinari changed the title [Sugestion] Change style engine [Sugestion] Change style engine to styled-components May 9, 2017
@followbl
Copy link

@followbl followbl commented May 28, 2017

@mxstbr thanks for styled-components

This works as long as the components attach the className prop internally to some DOM node

this might be worth highlighting somewhere in your usage guide when mui:next is released. This comment just saved me.

@yhaiovyi
Copy link

@yhaiovyi yhaiovyi commented Nov 9, 2017

Flex styles for IE10 don't work with jss but work with styled-components like a charm

@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Nov 9, 2017

@yhaiovyi Material-UI doesn't support IE10.

@kof
Copy link
Member

@kof kof commented Nov 9, 2017

Vendor prefixer evtl. Will be fixed soon for jss, doesn't mean though it will fix all issues if mui was never tested on IE10

@yhaiovyi
Copy link

@yhaiovyi yhaiovyi commented Nov 10, 2017

Anyway I haven't found any other problems then css flex with IE10 so far

@Danilo-Araujo-Silva
Copy link

@Danilo-Araujo-Silva Danilo-Araujo-Silva commented Jan 10, 2018

Looks like we have 3 ways (could be easier, but not everything is flowers) to override Material UI styles with Styled Components. Here is my Gist.

@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Jan 11, 2018

You can also get a styled-components API like with few lines of code: https://material-ui-next.com/customization/css-in-js/#styled-components-api-15-lines-

@caub
Copy link
Contributor

@caub caub commented Mar 11, 2018

You can also use styled-jss as well, example: https://codesandbox.io/s/32mvjyjyxq

the only downside with JSS in general is the lack of autocompletion in code editors, like said here too, but the benefits are there, parsing css to js like in styled-components is a bit an overload

edit: just noticed the referenced issue just above, interesting

@caub
Copy link
Contributor

@caub caub commented Mar 13, 2018

What's annoying is Mui's context and withStyles HOC don't seem to play well with the core react-jss and styled-jss ThemeProvider https://codesandbox.io/s/32mvjyjyxq (I tried putting a Typography but that doesn't work, edit: nvm, still fiddling with it)

I wonder if later (post v1 I guess) it wouldn't be worth to simplify src/styles/withStyles and MuiThemeProvider + JSSProvider double layer, and have something a bit more simple like how react-jss and styled-jss have

@kof
Copy link
Member

@kof kof commented Mar 13, 2018

@egilsster
Copy link

@egilsster egilsster commented Apr 22, 2020

Might have missed this answered somewhere, but want to raise it anyway. We're in the progress of migrating to Material UI components as well as evaluating our styling solution (less vs makeStyles vs styled-components), most of our styling is done through less files and most of the new MUI code is using makeStyles, what I want to know clarify is since this will be part of v5, what can we do to reduce the technical debt around styling?

  1. Will makeStyles be around in v5? How will it work with styled-components and themes? For example, how will this look with styled-components:
const useStyles = makeStyles((theme) => ({
  paper: {
    padding: theme.spacing(2),
    textAlign: 'center',
    color: theme.palette.text.secondary,
  },
}));
  1. (v4) Is there a better way to retrieve the theme object in a styled template string than this, so I can use it multiple times?
const StyledContainer = styled.div`
  color: ${({ theme }) => theme.palette.primary.main};
  padding: ${({ theme }) => theme.spacing(1)};
  background-color: ${({ theme }) => theme.palette.background.paper};
`;
  1. Assuming makeStyles is present in v5, should we expect any performance hit if we're using both the styled-components theme provider and the Material UI theme provider for a large codebase?

Love using Material UI, thanks for all the hard work!

@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Apr 22, 2020

@egilsster

  1. Yes, either from @material-ui/styles or react-jss, will see.
  2. Yes https://material-ui.com/guides/interoperability/#theme,
const StyledContainer = styled.div`
  ${({ theme }) => `
  color: ${theme.palette.primary.main};
  padding: ${theme.spacing(1)};
  background-color: ${theme.palette.background.paper};
  `}
`;
  1. The main issues are 1. configuration overhead, one time cost, 2. loading two CSS-in-JS runtimes, which has a ~15 kB gzipped overhead, a cost similar, to say, including Sentry: https://bundlephobia.com/result?p=@sentry/browser.
@egilsster
Copy link

@egilsster egilsster commented Apr 22, 2020

Thanks for a quick reply, very appreciated.

  1. Yes material-ui.com/guides/interoperability/#theme

This is pretty cool. Some work around tooling would probably follow this, the project I am migrating is not using TS but VSCode / language server does not seem to have any idea what theme is in this case and I lose styled-components syntax highlighting as well.

Thanks again, will continue following this development.

@jedwards1211
Copy link
Contributor

@jedwards1211 jedwards1211 commented Apr 22, 2020

@oliviertassinari if there's a migration to styled-components, will that mean we can no longer rely on CSS APIs like <ListItem classes={{ selected: myCustomClassName}}> to be around?

@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Apr 22, 2020

@jedwards1211 The classes API will stay.

@jedwards1211
Copy link
Contributor

@jedwards1211 jedwards1211 commented Apr 22, 2020

Okay. I'm a bit confused how MUI would use styled-components internally then, since styled-components can only apply a single class to the root element.

@yordis
Copy link
Contributor

@yordis yordis commented Apr 22, 2020

const MyRoot = styled('div')`
  // some nice styles
`;

const MyAwesomeChild = styled('div')`
  // some nice styles
`;

export function AwesomeRoot(props) {
  return (
    <MyRoot className={props.classes?.root}>
      <MyAwesomeChild className={props.classes?.child}/>
      {props.children}
    </MyRoot>
  );
}

Does that make sense @jedwards1211 ?

@jedwards1211
Copy link
Contributor

@jedwards1211 jedwards1211 commented Apr 22, 2020

@yordis I know it seems that simple, but how would you refactor compound selectors like https://github.com/mui-org/material-ui/blob/master/packages/material-ui/src/Button/Button.js#L75?

I really can't think of a way to preserve this existing behavior with styled-components, so it might cause more breaking changes than people expect.

  outlined: {
    '&$disabled': {
      border: `1px solid ${theme.palette.action.disabledBackground}`,
    },
  },

The existing code and people's custom overrides probably depend on CSS specificity in some cases too.

@yordis
Copy link
Contributor

@yordis yordis commented Apr 22, 2020

Maybe this? I am not sure to follow since this feels basic to me, I am probably missing some context and/or info.

const MyOutlinedComponent = styled('div')`
  ${props.disabled && `
      border: `1px solid ${({ theme }) => theme.palette.action.disabledBackground}`,
  `}
`;

<MyOutlinedComponent disabled/>
@jedwards1211
Copy link
Contributor

@jedwards1211 jedwards1211 commented Apr 22, 2020

@yordis possibly. As I said though people probably aren't thinking about how many breaking changes this will cause for users, I don't think that example would prevent breaking changes.

@yordis
Copy link
Contributor

@yordis yordis commented Apr 22, 2020

Would you mind sharing real examples, and real potential breaking changes?

It is hard to follow you when you don't share strong cases. Based on your messages I think you don't fully understand this topic, or I may be making wrong judgments. Please help me to understand, I may be the wrong one.

@jedwards1211
Copy link
Contributor

@jedwards1211 jedwards1211 commented Apr 22, 2020

@oliviertassinari will theme overrides still work without modification in v5? For the following override example to work, it seems like MUI would still have to apply JSS-generated class names in addition to a styled-components-generated root class name.

const theme = createMuiTheme({
  overrides: {
    MuiButton: {
      root: {
        '&$disabled': {
          color: myCustomColor,
        },
      },
    },
  },
});

@yordis having thought through it more, I realized compound selectors for styles passed via classes prop would still happen to work, luckily. Not sure about theme override styles though.

@Icehunter
Copy link

@Icehunter Icehunter commented May 5, 2020

I've had the pleasure of using both styled-components over MUI along with just plain MUI styled using using style={object}.

Take a result list page, that contains 50 cards, each with media carousels, info, click handlers, buttons, etc.

Using styled components added almost half a second to the render time; as compared to const useStyles = makeStyles or style={object}.

I'll have to accept the result of this roadmap planning; but it definitely will throw a kink in our plans for UI adoption if it's not overridable with something else completely top-down.

@yordis
Copy link
Contributor

@yordis yordis commented May 5, 2020

@Icehunter could you post your results and sample project online for people to look at it?

@Icehunter
Copy link

@Icehunter Icehunter commented May 5, 2020

@Icehunter could you post your results and sample project online for people to look at it?

Sample project would be hard as it would container proprietary code. I'll post a rendered image and the timings results section of the performance tab soon.

@tuxracer
Copy link
Contributor

@tuxracer tuxracer commented May 6, 2020

Take a result list page, that contains 50 cards, each with media carousels, info, click handlers, buttons, etc.

@Icehunter are you sending any props to those styles? whether it's 50 cards or 500 cards the number of classes generated should be the same. sounds like your specific example contains proprietary code that can't be shared but would it be possible for you to reproduce this problem with code you can share?

@schnerd
Copy link

@schnerd schnerd commented May 6, 2020

styled()/styled.div APIs add overhead to the rendering of every element, so even with caching of class names it can be slower. With makeStyles you can attach a group of styles once and then just apply class names manually, which is often significantly faster.

I put together a CodeSandbox example to illustrate that solutions relying on styled components styled can be 3-4x slower than an MUI makeStyles:
image

styled APIs are nice for convenience, but I echo @Icehunter's sentiment that it can be a performance concern when used heavily in lists/tables. It's nice to have makeStyles as a backup.

@tuxracer
Copy link
Contributor

@tuxracer tuxracer commented May 6, 2020

@schnerd Thanks for putting together these examples it illustrates the concern really well. Just as a side note think prefacing the post with, "It's not difficult to see..." can come off as kind of condescending and didn't really add to an otherwise excellent set of examples

@schnerd
Copy link

@schnerd schnerd commented May 7, 2020

@tuxracer apologies – updated.

@jedwards1211
Copy link
Contributor

@jedwards1211 jedwards1211 commented May 7, 2020

@Icehunter I don't understand what you mean by that, you used the common styles with SC or JSS?

@Icehunter
Copy link

@Icehunter Icehunter commented May 7, 2020

@Icehunter I don't understand what you mean by that, you used the common styles with SC or JSS?

Both actually. We ended up going with makeStyles but either that or using styled={object} got us the same perf results.

We are in a migration process to get better performance across the entire component library and main site.

Stack wise it's written in nextjs.

Just to be clear (edit):

I've used SC as intended, wrapping mui components. Turned out very slow.

Then I used mui components using makeStyles and/or style={object} from a shared file setup, and local (simulated cascading). Much much faster.

I don't want to block this idea at all; but if it is default; then there should be a way to override globally top-down the default choice and dep inject your own.

@yoohahn
Copy link

@yoohahn yoohahn commented May 8, 2020

Maybe this? I am not sure to follow since this feels basic to me, I am probably missing some context and/or info.

const MyOutlinedComponent = styled('div')`
  ${props.disabled && `
      border: `1px solid ${({ theme }) => theme.palette.action.disabledBackground}`,
  `}
`;

<MyOutlinedComponent disabled/>

Maybe im to late to this game. But I think @jedwards1211 is looking for a way of how this can be expressed with SC: https://codesandbox.io/s/magical-snow-5bzd8

I my self have this in some places. So would be nice if this was simple to migrate to v5 when that day comes

@JasonHK
Copy link
Contributor

@JasonHK JasonHK commented May 9, 2020

Actually I'm not sure how something like this will work using styled-components.

For example, if Material UI will support overriding the default variants of Typography in the future, I guess JSS is easier than styled-components.

@South-Paw
Copy link

@South-Paw South-Paw commented Jun 3, 2020

@heb-mm there is a detailed RFC here which @oliviertassinari mentioned this thread on the 7th of March.

It took me under a minute to scroll up and see the mention.

Edit: for those wondering, heb-mm has deleted their comment now.

@TheHolyWaffle
Copy link
Contributor

@TheHolyWaffle TheHolyWaffle commented Jun 12, 2020

I put together a CodeSandbox example to illustrate that solutions relying on styled components styled can be 3-4x slower than an MUI makeStyles:

@schnerd I've updated your benchmark to include the Material-UI inhouse styled api which should mimick styled-components. I was surprised to see how incredibly slow it is compared to the other options. See https://codesandbox.io/s/css-in-js-comparison-ljtjz?file=/src/App.js

image

@mbrowne
Copy link

@mbrowne mbrowne commented Jun 12, 2020

Does anyone know if @emotion/styled (which has basically the same API as styled-components) would be equally slow? I'm just wondering if there's anything about their implementation that might be better optimized.

@Icehunter
Copy link

@Icehunter Icehunter commented Jun 12, 2020

Does anyone know if @emotion/styled (which has basically the same API as styled-components) would be equally slow? I'm just wondering if there's anything about their implementation that might be better optimized.

https://codesandbox.io/s/css-in-js-comparison-sej1m

image

About as fast as styled components. Still not as fast as makeStyles. The issue I see for the most part is object creation and object caching/memoization differences.

@ldiego08
Copy link

@ldiego08 ldiego08 commented Jun 12, 2020

Hmm, this might trouble our migration plans to MUI quite a bit. We're currently moving away from styled-components in favor of the Material UI styling system after running into a lot of trouble managing our CSS, theming, and performance. styled components was fine at first, we built our own theming solution on top of it, but when the UI grew, so did the CSS. We use TypeScript, so the fact that we could easily refactor a JS object instead of an inline CSS string was a huge selling point. 😕

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.