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

Refactor CSS into Javascript #30

Closed
aranw opened this Issue Nov 10, 2014 · 95 comments

Comments

@aranw
Copy link

aranw commented Nov 10, 2014

Move component CSS into Javascript to remove the need for adding CSS/Less files to projects.

Suggest comes from seeing this slideshow https://speakerdeck.com/vjeux/react-css-in-js from @vjeux

@aranw aranw changed the title Possible CSS Improvement Refactor CSS into Javascript Nov 10, 2014

@totty90

This comment has been minimized.

Copy link

totty90 commented Nov 10, 2014

I like that, but I don't know how it works in real world. Imagine having a lot of elements, every element will have a custom style. It will make the dom huge. Isn't this a performance problem? (I've made a similar UI entirely in react.js with style and everything worked ok, but was a small test project)

@aranw

This comment has been minimized.

Copy link

aranw commented Nov 10, 2014

@totty90 yeah I did wonder this, but from the slides the issue at Facebook from sounds of it was that all the CSS files and CSS classes were a performance issue.

Maybe @vjeux could shed come light on this and its the suggested solution in that slideshow.

Maybe for some of the smaller components this might be a better solution?

Edit: Another thing I was unsure of was how to deal with media queries and responsiveness?

@totty90

This comment has been minimized.

Copy link

totty90 commented Nov 10, 2014

Already asked him to show me a real world app using this technique. This solves one of the best problems: a component came in a single file. I can share a file with you and you get my component rendered exactly as mine without any setup from your part. No css problems.

@robertknight

This comment has been minimized.

Copy link

robertknight commented Nov 10, 2014

There is another project which aims to provide a good way to integrate CSS-in-JS and React - https://github.com/js-next/react-style and also some initial work on Material UI components using them - https://github.com/SanderSpies/react-material

This project has more traction at the moment and a larger number of components implemented, but react-material does have the advantage that components work standalone without having to bundle CSS files. You also get all the benefits outlined in @vjeux's presentation from it.

It would be great if the two projects could be combined!

@totty90

This comment has been minimized.

Copy link

totty90 commented Nov 10, 2014

The other project is a lot worst. The only part interesting is the css in js.

@pspeter3

This comment has been minimized.

Copy link

pspeter3 commented Nov 10, 2014

I like the concept of having JS and CSS be coupled but inline styles are bad for browser performance and implementing CSP. Maybe use WebPack if you're concerned?

@jarsbe

This comment has been minimized.

Copy link

jarsbe commented Nov 10, 2014

Webpack seems to be the only "reasonable" way to import styles. Have a require('component.css'); - but then you end up having a dependency for your component. They must use Webpack too! I don't like that solution. One should be able to share a component easily just install, use and profit.

The current approach here looks user friendly. It's not perfect, but copying a couple of import lines is quite simple. Although I would suggest that using Less once again locks users into that preprocessor...

It's tough making things simple! Anybody got any suggestions? This is much wider than just this project. The entire react/polymer/component community needs a solution.

P.S - coupling CSS inside your JS looks like a bad idea. For one you end up writing a pseudo CSS with camel cased properties and stringified values and you are cognitively mixing concerns making it harder to reason about your code. I've never had issues writing CSS. Yes, it can be hard and things can go wrong (global namespace, unintended consequences etc) - but shifting that to JS is NOT going to fix the issue. Writing better CSS is.

@vjeux

This comment has been minimized.

Copy link

vjeux commented Nov 10, 2014

@pspeter3 do you have any benchmarks? :)

@pspeter3

This comment has been minimized.

Copy link

pspeter3 commented Nov 10, 2014

Mainly these two JsPerfs, http://jsperf.com/class-vs-inline-styles/2 and http://jsperf.com/classes-vs-inline-styles. However http://jsperf.com/inline-style-vs-css-class/9 does seem to imply the opposite. Bad for performance was an overstatement. I'm sure you can also cause bad performance with stylesheets by using poor selectors. Do you have any advice on implementing Content Security Policy when using inline styles?

@totty90

This comment has been minimized.

Copy link

totty90 commented Nov 10, 2014

@jarsbe

P.S - coupling CSS inside your JS looks like a bad idea. For one you end up writing a pseudo CSS with camel cased properties and stringified values and you are cognitively mixing concerns making it harder to reason about your code. I've never had issues writing CSS. Yes, it can be hard and things can go wrong (global namespace, unintended consequences etc) - but shifting that to JS is NOT going to fix the issue. Writing better CSS is.

I've written a lot of CSS and CSS into JS. The first one is easier but in big projects get messy the other one I think (because I've only built a small app) would work better in a bigger app. Try to program a js app with everything global + merging stuff, that's hell!!! (Or I've always done it wrong...)

Here is a test I've done with react.js http://jsperf.com/react-class-name-vs-inline-css2

@hai-cea

This comment has been minimized.

Copy link
Member

hai-cea commented Nov 11, 2014

Yes, I wish there was an easy answer for this. :) In the end, I think there are merits to both approaches.

My biggest concern with including styles inside the JS components has to do with code maintenance. For example, what happens to the CSS that's needed for cross browser resets? Do those styles go into the components as well? Also, we get a lot conveniences from using LESS like mixins, variables, etc that we would have somehow replace and abstract inside JS.

With that said, we've namespaced all of the CSS classes that are used in this project with "mui" and we try to write object oriented CSS. In the future, there probably should be some process that will let users pick and choose which components to download into a custom build.

Thanks for the issue. Really good points brought up is discussion!

@mcwhittemore

This comment has been minimized.

Copy link

mcwhittemore commented Nov 11, 2014

What really makes the style of a component less a part of that component than its structure (html) and functionality (js)? It seems to me that we've come to mistake a method to mitigate one problem (maintainability of code) as an inherently wise design pattern and have forgotten that it was really a patch. Arguably React is more elegant solution to the maintainability of code problem than dividing structure, functionality and style into their own languages. It has its own problems (speed in the css case), but if we come to agree that this reunited way of writing components is good we can start trying to solve the speed problem.

It should be noted that I personally think that this is one of the failures of the web components spec. Where it groups different languages into one module, React challenges the very idea of html and css.

@jarsbe

This comment has been minimized.

Copy link

jarsbe commented Nov 11, 2014

@mcwhittemore Yes, the style is no less a part of the component than the HTML or JS. Together they combine to create one component. This does not necessitate that the component should be written as one 'unit', that's just how it should be distributed.

React tightly couples the structure and functionality. It forces you into this pattern and I've found this to be simple and logical. I can easily understand the code I'm looking at, my brain is not overloaded.

On the other hand CSS inside a JS file makes the code harder to understand for me. Descriptions of visual styling do not fit well next to functionality and structure. I'm not simply annoyed by it I am confused and slowed down since I have to process multiple concerns. It also tightly couples the styling with the component which makes it harder for other authors to write their own styles.

I want to distribute my component as a single bundle. Lego block development is the dream! It sounds like we are all on the same page with that end goal. Inline CSS sounds like a good fix but I do not believe it is the right fix.

@mcwhittemore

This comment has been minimized.

Copy link

mcwhittemore commented Nov 11, 2014

@jarsbe 100% agree that reading style in the component is not clear enough. Simply finding where the style is created can be difficult at times. I've been using a style.json file where its defaults and permanents are merged with this.props.style does a bunch for clarity.

{
  "defaults": {
    "backgroundColor": "#efefef"
  },
 "permanents": {
    "width": "100%"
  }
}

Another argument against doing the styling in JS vs CSS is that the style attribute does not have the same powers as a CSS selector. For instance I don't think the *:after in the current less is possible to implement.

@jarsbe

This comment has been minimized.

Copy link

jarsbe commented Nov 11, 2014

@mcwhittemore trying to get a psuedo element was exactly what made me go, "huh? oh yeah that doesn't exist in this context...". Yes, extraction into another file does help. I must take a look at the system on bespoke.js for styling new themes. I've heard that implements CSS in JS but am weary.

@totty90

This comment has been minimized.

Copy link

totty90 commented Nov 11, 2014

@mcwhittemore why do you need *:after?

@mcwhittemore

This comment has been minimized.

Copy link

mcwhittemore commented Nov 11, 2014

@totty90 Its currently part of the CSS. There might be ways to solve this problem, but it is not a simple "convert your css to js" situation. Do you have a solution for the :after or :before selector that I'm just not thinking of?

@totty90

This comment has been minimized.

Copy link

totty90 commented Nov 11, 2014

IMO :after and :before are kind of hacks and I never needed them, I want to know what you need them for to give you a solution/opinion.

@mcwhittemore

This comment has been minimized.

Copy link

mcwhittemore commented Nov 11, 2014

@totty90 I'll mention you in another issue on another project so not to clutter this one.

@WRidder

This comment has been minimized.

Copy link
Contributor

WRidder commented Nov 12, 2014

@hai-cea Keeping a SMACSS workflow in mind (https://smacss.com/book/categorizing) a good approach may be to keep the css for the Base, Layout and (maybe) Theme categories in the good ol' css files. Module and State css seems like a good candidate for me to do JS style.

@hai-cea

This comment has been minimized.

Copy link
Member

hai-cea commented Nov 14, 2014

@WRidder Thanks for the link - that makes a lot of sense.

@jarsbe

This comment has been minimized.

Copy link

jarsbe commented Nov 19, 2014

A random thought that I didn't know where to put... How do native components implement styling? I envision that react components should be as similar in nature to native components. It might be a good place to look for ideas.

@nigelsmith

This comment has been minimized.

Copy link

nigelsmith commented Dec 2, 2014

I'm very sceptical of this approach of placing CSS within JS - the presentation itself talks about the issue of associating styles with components as if there hasn't been an entire, multi-year long, effort to do just that with the Shadow DOM standard. Now clearly React doesn't use that mechanism at present but I wouldn't be surprised if it did in time.

That standard works because it provides a way for a component creator to associate some minimal set of basic styles with their component that can then be overridden by a designer reaching down into the shadow DOM. It's far from clear how a designer would override styles set purely within Javascript if they weren't themselves using the components with react directly.

@hai-cea

This comment has been minimized.

Copy link
Member

hai-cea commented Jan 31, 2015

I just got back from the react conf and thank you so much to @vjeux for organizing it. I was skeptical with this approach as well, but I think it does have its advantages. This project has some challenges that I'd love some help solving if we went with this approach.

  1. We'd like these components to be themeable. They would have some default styles, but developers should be able to change the look to suit their needs. Currently, they can do this by overriding certain variables in LESS. How could we offer the same capabilities using js styles?
  2. A lot of times, components will generate nested children. How do we let developers customize styles on nested children. My instinct is that we should only expose style props on those children that would be customizable? But would this tightly couple our styles with a component's dom structure? What I mean is, if we end up changing a component's dom structure, would we end up changing it's style props as well?

Thanks for all of your help and feedback so far - I'm looking forward to what everyone has to say.

@pspeter3

This comment has been minimized.

Copy link

pspeter3 commented Jan 31, 2015

Was there a video about this idea at the React conference?

@hai-cea

This comment has been minimized.

Copy link
Member

hai-cea commented May 27, 2015

Thanks to everyone for this awesome discussion. We were able to release this feature in v0.8.

@pablofierro

This comment has been minimized.

Copy link

pablofierro commented Aug 23, 2015

I was looking forward to using this library but seeing everything uses inline styles was a huge turn down, I do not understand why you would do this, I read the slides on why it was done but it makes no sense, why merge appearance, views and logic into one single thing, separation of concerns people, that's one of the most solid statements for software engineering.

@wmertens

This comment has been minimized.

Copy link

wmertens commented Aug 23, 2015

Pablo, these concerns are not separate. If you want things to look
differently you can use the theme manager, a true separation of concerns.
For the rest, what components should do and their html and css are closely
coupled, and separating that out just makes things more complex.

On Sun, Aug 23, 2015, 05:35 Pablo Fierro notifications@github.com wrote:

I was looking forward to using this library but seeing everything uses
inline styles was a huge turn down, I do not understand why you would do
this, I read the slides on why it was done but it makes no sense, why merge
appearance, views and logic into one single thing, separation of concerns
people, that's one of the most solid statements for software engineering.


Reply to this email directly or view it on GitHub
#30 (comment)
.

Wout.
(typed on mobile, excuse terseness)

@grrowl

This comment has been minimized.

Copy link

grrowl commented Aug 27, 2015

I agree with @pablofierro — inline styles can't be cached by the browser and we lose fairly basic CSS functionality like :hover, sibling selectors or any notion of cascading styles.

It's a compromise to go with inline styles to enable static client-side-only apps and avoid dependencies on pre-processing toolchains (webpack, etc), but it lets down larger projects with the extra capability (isomorphic apps most prominently). I'd love the option using external stylesheets or use of CSS Modules support but it'd screw over the simpler cases.

@Kagami

This comment has been minimized.

Copy link

Kagami commented Aug 27, 2015

@grrowl cascades are considered bad practice in large stylesheet codebases anyway. See e.g. BEM approach.

@pablofierro

This comment has been minimized.

Copy link

pablofierro commented Aug 28, 2015

@Kagami considered bad practice by whom ? If you break your stylesheets into reusable modules(similar to the react philosophy) this becomes highly maintainable. Even React's webpage mentions it is the View for your components and not the appearance, this decision of forcing everything to be inline is just plain ridiculous.

@rozzzly

This comment has been minimized.

Copy link

rozzzly commented Aug 30, 2015

This needs to be changed. Inlining certainly has it's benefits in some use cases, but the resulting markup is:

  • bloated -- a ton of bandwidth is gonna be used up just to send down the style. In every request. For every element node... Just a quick estimate--I don't care to spend any time doing the metrics--here, but it would not be unreasonable to assert that the resulting markup could be 5x LARGER for some run of the mill app. And what if you have an app with many, many DOM nodes? And how does that affect memory overhead and performance? This brings in a whole host of implications that we shouldn't have to deal with.
  • ugly -- have fun reading what it generates, its about as enjoyable as reading raw/unformatted minified css.
  • and stupid -- all the way back to the CSS 1 spec, the WW3C included classes for a reason. This approach styling throws the flexibility/rapid prototyping/functionality the community has gained by creating LESS/SCSS/etc. etc. out of the window. You can already see the wheel being reinvented; scroll up though this thread and look at all the comments suggesting different libraries/whatever for extracting the css from javascript.

But wait! There's more:

  • No out of the box IDE support
  • No psuedoclass support

Guys: this is something jquery can do. Why do we have to dredge this up to the surface? moreover force it by default?

a fun case-in-point

Open Chrome Developer Tools
Try making some style edits--change the background color or something--to a page that has several mui components of the same type, e.g. the <MenuItem ...>'s in a navigation menu.

it only changes that component. What about React Devtools? nope same thing.

I don't mean to be a dick--although I most assuredly sound like one, but as @pablofierro said, this is ridiculous. This functionality may be cool, the lack of css dependencies is very appealing. But ripping out the .css doesn't make sense.

@rozzzly

This comment has been minimized.

Copy link

rozzzly commented Aug 30, 2015

I should add; I'd be more than happy to create a PR. I just want to hear what the community has to say about reimplementing this; hopefully come to some kind on consensus on how to go about this.

@oliviertassinari

This comment has been minimized.

Copy link
Member

oliviertassinari commented Aug 30, 2015

What do you suggest to do instead? For my use case (Cordova app) the inlined approach is relay great.

@DylanPiercey

This comment has been minimized.

Copy link

DylanPiercey commented Aug 31, 2015

@oliviertassinari it seems like the biggest benifit of inline styles is that the project does not need to have an external style sheet as a dependency.

If this is the case, I would recommend reverting back to classes (as it seems many want that and it seems to make things simpler) and instead exposing all of the CSS via a string.

/*styles.css for people who are fine with a separate file*/
.my-styles {...}
// styles.js (expose as "material-ui/theme" or something)

var css = { __html: "injected styles.css content" };

// alternatively we could expose a component.
module.exports = (
    <style dangerouslySetInnerHTMLIfYouAreSureItIsWhatYouWantToDo={ css }/>
);

This way people can use the theme directly with JavaScript by:

theme = require("material-ui/theme");

MyJSX = (
    <div>{ theme }</div>
);

Pros:

  • its just css
  • potentially multiple themes without it all being bundled
  • can be imported through js, css pre processor or manually in html.

Cons:

  • a bunch of time was spent making all styles inline.
@bulby97

This comment has been minimized.

Copy link
Contributor

bulby97 commented Sep 4, 2015

@rozzzly +1111111000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000

With MUI -> http://recordit.co/9mBknPLZUb
Without MUI -> http://recordit.co/6MkzWhQiBt

@oliviertassinari

This comment has been minimized.

Copy link
Member

oliviertassinari commented Sep 4, 2015

Well @rozzzly I disagree.

  • bloated. Sure, we use more bandwidth to send the style when the user arrives in the site.
    But latency and not bandwidth, is the performance bottleneck for most websites. for more detail.
    So, I think that sending the style inlined lower down the first render time. And this is what really matters for users. You don't have to wait the RTT (round-trip time) to have the style.
  • ugly. Yeap, it's not easy to read. Why do you need to read it?
  • and stupid. Facebook explained their motivations for using inlined style. It's pretty convincing. The link
@iclanzan

This comment has been minimized.

Copy link

iclanzan commented Sep 4, 2015

I’ve been using inline styles exclusively for more than 6 months and it has been a liberating experience. Styles have become more manageable, flexible and with far less surprises compared to CSS.

@adamscybot

This comment has been minimized.

Copy link

adamscybot commented Oct 15, 2015

I personally agree with @rozzzly who details the issues well. This just isn't a good idea at scale. CSS works. We're reinventing the wheel just so its more "portable". This insane change boils down to fixing one problem -- including css. This would take 1 line of explanation in the readme. Not to mention, even rookie developers are well versed in including CSS in HTML or with their build tool. This isn't new people.

Instead we're rebuilding styling for web pages in JS. It just seems silly. Absolutely basic styling features like pseudo elements? Gone. SASS/LESS compilation? Gone.

@mpseidel

This comment has been minimized.

Copy link

mpseidel commented Oct 28, 2015

Hey everyone. I started using material-ui in a side project and have been following the discussion here and here.

I wanted to add a few points to the mix:

Does this make sence for smaller applications?
I have written a lot of smaller internal web apps for businesses using bootstrap and the like. I have never felt much pain when having to include some more component-css alongside its markup and js. I'ts a one time effort. Conflicts were very seldom an issue and easy to resolve most of the time.

But I defenitely feel pain looking at the DOM in my devtools now. Even seeing the basic tree structure is much much harder with all the inline styles. And this pain is not a one time thing - its coming back everytime I open up devtools.

Are we alienating CSS-savvy designers here with this approach?
With all the styles embedded in JavaScript it seems hard for designers that are used to draft and edit working in CSS files. Aren't they now forced to learn JavaScript, ES6, transpilers, buildtools etc and are much more likely to break stuff?

Maybe thats not a bad thing - I am all for cross functional teams and I haven't worked with "component designers" yet, but I want to make sure designers get considered in this discussion as well.

Also:

+1 for separation of concerns - styling and ui logic have to be separated in my book
+1 for considering lack of IDE-Support

I absolutely agree that there are problems with CSS especially at scale. I hope this discussion leads to a better place for all of us in this regard.

@wmertens

This comment has been minimized.

Copy link

wmertens commented Nov 24, 2015

Well, I too found the talk convincing but in practice using mostly css with
module-based renaming and maybe some inline styling is what is easiest to
work with imho.

On Tue, Nov 24, 2015, 9:49 AM Owen Campbell-Moore notifications@github.com
wrote:

For what it's worth, use of inline styles was a large reason I became a
huge fan of MUI. I found the talk by Facebook extremely convincing and
personally feel that style and structure do not make sense to be separated
when building UIs.


Reply to this email directly or view it on GitHub
#30 (comment)
.

Wout.
(typed on mobile, excuse terseness)

@justjacksonn

This comment has been minimized.

Copy link

justjacksonn commented Dec 2, 2015

Interesting points @mpseidel. I however think as more and more frameworks move towards components, it seems to me keeping the JS and CSS together as part of the component makes more sense than having a more global css include that covers a range of components, pages, etc. However, the concept of theming components has me scratching my head.. I don't know CSS much, so maybe there is an easy way for components to pick up theming styles to overwrite their default styles so that component writers can have their components fit in with an overall theme style? If there is I'd like to understand that more.

@akaRem

This comment has been minimized.

Copy link

akaRem commented Dec 10, 2015

There is a big discussion here. And It's hard to guess overall direction.
Can anyone estimate the chances that styles will be returned to their place?

I don't use material-ui yet. And from my "fresh" point of view, I see these problems which beware me of using this project in future:

  • presentation and logic layers are messed
  • performance degradation due to inline styles
  • hard to add own styles
  • Ugly unreadable DOM

I don't see any benefits from inline styles.

It would be better to reorganize less/sass/css in more readable and maintainable way than try to reinvent styling from scratch.

@oliviertassinari

This comment has been minimized.

Copy link
Member

oliviertassinari commented Dec 10, 2015

This discussion is pretty much finished.
Please follow #684.

@akaRem

This comment has been minimized.

Copy link

akaRem commented Dec 10, 2015

@oliviertassinari .. it means no way back to CSS?

@oliviertassinari

This comment has been minimized.

Copy link
Member

oliviertassinari commented Dec 10, 2015

It means that we are exploring options to improve the current style approach (CSS Modules, Radium, Looks, react-themeable).

@mui-org mui-org locked and limited conversation to collaborators Dec 10, 2015

@mui-org mui-org unlocked this conversation Apr 22, 2018

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