-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[RFC] Migrate to styled-components #6115
Comments
This comment has been minimized.
This comment has been minimized.
@kybarg Actually, I'm not sure to fully understand what you are suggesting. When you say we, are you talking about users or Material-UI? |
My points are:
|
That table is indeed very subjective and based on my own experience. FWIW, 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 const MaterialUIButton = (props) => {
/* ... */
// As long as props.className is attached, the above usage will work
return <button className={`bla ${props.className}`} />
}
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 I've been talking to @javivelasco for a while now and love where he's going with
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. |
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. 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 |
Somebody has to lock this thread. @rainice jss doesn't have decorators, a HOC is an implementation detail of react-jss and is not used here. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I wish we could have moved forward that thread with a less coupled styling solution! |
@mxstbr thanks for styled-components
this might be worth highlighting somewhere in your usage guide when mui:next is released. This comment just saved me. |
Flex styles for IE10 don't work with jss but work with styled-components like a charm |
@yhaiovyi Material-UI doesn't support IE10. |
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 |
Anyway I haven't found any other problems then css flex with IE10 so far |
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. |
You can also get a styled-components API like with few lines of code: https://material-ui.com/customization/css-in-js/#styled-components-api-15-lines- |
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 |
What's annoying is Mui's context and I wonder if later (post v1 I guess) it wouldn't be worth to simplify |
Totally for it!
…On Mar 13, 2018 13:55, "Cyril Auburtin" ***@***.***> wrote:
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 wonder if later (post v1 I guess) it wouldn't be worth to simplify
src/styles/withStyles and MuiThemeProvider + JSSProvider double layer
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#6115 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADOWAbwLOnRoypx9ANCZnKyalZyD0M9ks5td8HNgaJpZM4L-GwD>
.
|
@martinjlowm Yep, we are on the same page :) I think that the smartest way to go for the material UI team is to implement some kind of pluggable solution and not couple with any css in js solution, letting people choose what to use and save some bundle size that way. And also keeping classes and createMuiTheme API, because that's the most powerful part of it. For me, css in js is about easy dynamic styling based on component state, confidence when refactoring or deleting css or whole components (that's where object approach is superior), performance (not downloading a bunch of unused styles from a bunch of external stylesheets), etc.. And again, I don't understand why people like writing string with some editor highlighting. I mean, you have to use ${} to access props from context. For anything more complex than setting background od div element, that approach is messy and not readable in my opinion. I am not bashing it, just saying what I think and trying to prove a point that not every developer like styled-components, and even if they did, I don't see how it is a good deal to trade performance and flexibility for that :) |
@vdjurdjevic Indeed, coupling material-ui to one css in js solution is almost a guaranteed recipe for conflict when css-in-js practices inevitably grow in different directions. How about some adapter pattern that applies styling with a specific css-in-js provider? The styles themselves should be defined in a consistent format within the material-ui packages. It's the applying of those styles that is performed differently by the corresponding adapters. Something along the lines of:
|
@vdjurdjevic I pretty much agree with your thoughts on #6115 (comment). You might like this summary of the direction we are taking that I have recently shared in #16947 (comment).
A couple of reasons from my perspective:
|
@oliviertassinari Ok, these are some solid points, I agree :) But still, for me (not a designer, not a beginner, senior developer), styled-components and tagged template literals would never be an option. I read your summary for the direction of further development, and I am happy to hear that css engine will be optional. I don't mind styled-components as a default (if that's what the majority of users like), as long as I can switch that. And to be honest, I think I will stick to JSS with v4 for my next project, it has some nice features (like expand and compose plugins). I had to write stylis plugin for emotion to have something similar. PS. I am not big of a fan of JSX either :) It quickly gets messy, you end up with arrow code and for components that should render dynamic elements, you have no choice but to use createElement. Not saying that working directly with createElement is better, just saying that JSX is not ideal. In my opinion, Flutter has the best DX, I hope it will be able to handle web platform well someday. |
@oliviertassinari Per our ongoing perf testing as noted in #6115 (comment) I just hope the material team doesn't just choose styled components because it's popular. It's slow. Always has been. Personally it's a point of being a dev that you need to learn things like JS notation of CSS. It's part of the job. Making devs lives easier is part of our job as package maintainers (for my own projects I'm speaking from) but there comes a line between making it easy and making it performant. It's why I only use makeStyles and always have since it was introduced. My last teams architect wouldn't listen and plowed ahead with styled components and now the site is slow. I switched out to makeStyles in a test branch and saved 50% (10 seconds) on TTI on mobile devices, but just like you said in that comment, JS notation isn't known by everyone so it wasn't accepted. Internally material can choose whatever it wants but please make it performant by default. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I was wondering what the performance of the Box component would be compared to your profiling, and added Chakra-UI's Box for good measure, and the results were... surprising :P |
@Nvveen have you tried Chakra-UI v1? It's been rewritten and hopefully should be better. There's also emotion v11, although still in beta. |
Just tried it with the latest RC, but no noticeable difference; it's still about 1.5x to 2x as slow as regular SC. Biggest question to me is why the MUI Box is so slow. |
It uses Jss. SC is at least somewhat better. |
On my machine, the order in which the test cases are run impact the results. Compare |
@oliviertassinari garbage collection and jit from v8 is what's causing it. Better to have test runs isolated / separated really. The later tests have better jit, but once in a while it'll trigger garbage collection. |
It looks like Emotion, SC, and Chakra are all close to a theoretical maximum perf for when each little component has its own styles to mount, with MUI styled not quite fully optimized yet. Seems like the only way to get better perf than that is to have one style sheet for the whole component tree as in makeStyles/pure CSS. I'd guess the additional 30 ms for emotion, SC etc. is the unavoidable overhead of each little component having to make sure its styles are mounted. |
@jedwards1211 if it's about keeping the dx, Atlassian made a lib that compiles it to css, called compiled |
I'm closing for #22342. We are migrated to emotion by default, with a simple way to swap the engine to styled-components. Thanks all for the discussion, it took us 4 years, we are really close now: #24405. |
@oliviertassinari Could you please add an example to the repo covering Next.js with the styling engine swapped to styled-components? I just updated to MUI v5 and I'm having a hard time figuring things out. |
@CloutProject Adding an example with Next.js + Styled components sounds like a great idea. It now growing faster than create react app, the only tool for wish we have an example. Our approach is the same as Preact, so you could start from the official example. |
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!
Legend: ✅ = Yes, ❌ = No, 😕 = Kinda, refer to notes or parentheses
The text was updated successfully, but these errors were encountered: