mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
91.94k stars 31.61k forks source link

Consider using Radium for inline styles #684

Closed wmertens closed 8 years ago

wmertens commented 9 years ago

https://github.com/FormidableLabs/radium adds "interactive" styles (hover etc) to inline styles, and also allows hierachical styles and automatic vendor prefixing. It's fast and pretty light-weight (32KB minimized).

sunkant commented 9 years ago

A good suggestion. Any one shed view on this?

ericfong commented 8 years ago

Tried 0.8. Found component rendered withOUT vendor prefixing in server but WITH vendor prefixing in clients which generate warning for every page. Hope mui can just generate all vendor prefixing, Thanks

wmertens commented 8 years ago

Yes, vendor prefixing in radium is only client-side to keep the code small and fast.

Server-side prefixing is hard to get right and there are good projects that do that already, with up-to-date databases of things that need prefixing.

On Wed, Jun 3, 2015, 6:36 PM Eric Fong notifications@github.com wrote:

Tried 0.8. Found component rendered withOUT vendor prefixing in server but WITH vendor prefixing in clients which generate warning for every page. Hope mui can just generate all vendor prefixing, Thanks

— Reply to this email directly or view it on GitHub https://github.com/callemall/material-ui/issues/684#issuecomment-108515460 .

ericfong commented 8 years ago

Better to have same output from server and client. Otherwise, react in client will drop the server render output (as I know).

wmertens commented 8 years ago

Doing proper prefixing means keeping a database of all necessary prefixes. Furthermore, adding lots of prefixes means extra memory and cpu usage (4x) for absolutely no reason. Keep the heavy prefixing to the server side and support minimal auto prefixing client-side.

On Thu, Jun 4, 2015, 5:58 AM Eric Fong notifications@github.com wrote:

Better to have same output from server and client. Otherwise, react in client will drop the server render output (as I know).

— Reply to this email directly or view it on GitHub https://github.com/callemall/material-ui/issues/684#issuecomment-108711459 .

SOSANA commented 8 years ago

Awesome just what I was looking for :)

hai-cea commented 8 years ago

Thanks @wmertens - I think Radium is looking really good. I can see us moving away from doing our own inline style merging and prefixing and transition to use Radium.

neverfox commented 8 years ago

At the risk of starting a flame war, is CSS Modules a possible contender?

oliviertassinari commented 8 years ago

I may be wrong, but I feel like CSS Modules is really closed to what Radium or react-look are providing. I'm also wondering how we would implement the RTL support with the CSS Modules approach, I think that it would be far less straightforward.

neverfox commented 8 years ago

Good point (though there is rtlcss for postcss). What's your opinion about react-look vs. Radium? On paper, it looks like it ticks more boxes.

oliviertassinari commented 8 years ago

At this point, I don't have any opinion. However, whatever we choose, I think that we need the following features:

This link could help https://github.com/FormidableLabs/radium/blob/master/docs/comparison/README.md

@shaurya947 @rofrischmann @ianobermiller @javivelasco would you mind give your point of view on what's best and why? So we can make an informed choice. Thanks.

wmertens commented 8 years ago

Actually, debugging is easier with css modules since you can just play around with the class in the css inspector. With inline styles your components are somewhat harder to read and all changes have to go through the save-update cycle.

Css modules work fine together with inline styling for the rare occasions when classes are not dynamic enough.

For me, the autoprefixing and debugging are the strong points of css. In react-native, neither are necessary and inline styles are (apart from being the only option) great.

robinweser commented 8 years ago

First of all I'd like to say that I never had the pleasure to test CSS Modules which means I only know theoretically what's possible. Seems like the main question is Inline Styles vs. CSS (Modules) as I'd say both Radium and Look would do the Inline Job well.

Disclaimer: I am a huge fan and supporter of Inline Styles because of flexibility and dynamic behavior. So this might be opinionated.

API Compatibility

I am also not familiar with your existing style API, but as you're actually using Inline Styles I'd assume that the compatibility should not be the problem if you choose Inline Styles (Can't speak for CSS Modules).

Pseudo Styles, Media Queries and Syntax

While Radium, Look and CSS Modules all support pseudo styles, media queries and a CSS/Sass-like syntax on client side, you might need some extra configuration to get those working on server-side if choosing Inline Styles, but it's nothing impossible either.

Dynamic behavior

Talking about dynamic styling there's a huge difference. While with Inline Styles you're free to do whatever you want, CSS Modules still render to static CSS which means having e.g. props or state depending styles is not quite possible (Therefore this might be solved by using CSS Modules for static styles & Inline Styles for dynamic styles). In my opinion the dynamic behavior is the most important advantage of Inline Styles as especially within React Components your styles quite often depend on the components props or state (or context) and I really like the idea of styles as a result of Component conditions and inner-application state. This connects the UI logic with the UI appearance.

Extracting CSS

This is trivial with CSS Modules as it actually does this automatically. Extracting your Inline Styles to static CSS files would drastically lead to disadvantages (in my opinion) as you would immediately loos flexibility and dynamic behavior.

JavaScript vs. CSS

While CSS is just a descriptive StyleSheet language, JavaScript is a powerful programming language that allows you to quite anything with your styles.

Autoprefixing While @wmertens mentioned autoprefixing as an advantage of CSS I'd actually say the advantage goes to Inline Styles as you're able to evaluate the userAgent and only add needed prefixes instead of adding every prefix (which is a huge superset of what is actually needed => bandwidth)

Summary

CSS Modules win at extracting CSS, caching styles and developer experience. There's also an advantage on server-side as everything works just as expected. Inline Styles win at flexibility, dynamic behavior and provide the possibility to use JavaScript which adds a lot of features CSS is not capable of.

If choosing CSS Modules you might consider using them in cooperation with Inline Styles as @wmertens mentioned above. If choosing Inline Styles you should do just fine with both Radium or Look. (Both more or less do the same thing in a similar way. They even use the same prefixer.)

Additionally some information on Look especially for your use case:

Well, that's it. I hope I covered everything. Happy reading :P And keep in mind that this is still my own opinion as creator of Look. Feel free to contact me if you'd like to have additional information or other questions according that topic.

neverfox commented 8 years ago

Despite bringing them up, I'm not a biased proponent of CSS Modules. In fact, I still think I'm more comfortable with inline styles because I understand them better. However, to be fair, I want to point out that it's not quite true that CSS Modules take away the ability to be dynamic with styles in the code. After all, you still have to decide, in the code, which classes to apply. It just requires a different approach. That's even true of global CSS (after all that's why people write different classes), but because the CSS is always scoped locally, it theoretically should make it easy to reason about that logic. It would just mean writing different classes for the different style states and choosing them with something like classnames in the component. That said, there still may be things that are impossible or difficult outside of Javascript, but it's trivial to throw those things into the style property when you have to.

robinweser commented 8 years ago

@neverfox Well yeah, that's true. You can create multiple classes for different states, but you still need to predefine them which also assumes that every value is yet known while building. After you're build step there are just static CSS rules that should not be modified later on. But you're right. This can be easily solved by adding some style next to the className if e.g. a user input is required to determine the correct styling. Mind that this could even be more confusing.

And don't get me wrong: I still think CSS Modules is totally capable of this job. It's a great project with a great community and awesome contributor behind.

oliviertassinari commented 8 years ago

@rofrischmann Thanks for this great comment! I agree with your analyse of the different options. CSS Modules and inline style have both advantages. If we had to start this project from scratch, I think that we should choose the inline style approach for a simple reason. Material-ui don't have to only target the browser. The inline style approach seems a better abstraction to also target react-native. We are far from this goal but I think that this makes us closer.

neverfox commented 8 years ago

I'll second the fact that @rofrischmann's comment was great.

javivelasco commented 8 years ago

Hi guys, first of all sorry for my late response, I've been so busy these days. I loved @rofrischmann effort writing his point of view and I think it's worthy to go with my opinion through the same bullet points. Also I'm adding a disclaimer:

I am supporter of CSS Modules to build web apps. In fact I made a proof of concept with React Toolbox. For me inline styles can complement CSS because of the dynamic behavior... but just that

API Compatibility

It looks like if you use a solution like Radium its no big deal to generate stylesheets. If you choose CSS modules I think switching is way more complicated. You may be able to generate an specific style object for Radium from CSS... but each IS solution has its own API. This leads me to give +1 to CSS modules because CSS is already an standard. IS libs have close APIs but they are not exactly the same.

Pseudo Styles, Media Queries and Syntax

You can't get rid of classic CSS to make your app work on the server side. CSS modules provides media queries and psedudo-elements/selectors. Although you can achieve it with Radium it's more difficult to do.

Dynamic behavior

Here Inline Styles are strong, but using CSS modules does not mean you can't use IS too. When you need dynamic behavior just add some inline styles. I think your UI depends on the state but usually a state refers to a bunch of CSS grouped on a class. Instead of providing styles when state changes just provide different classes. That's all.

Extracting CSS

No need to do magic with CSS Modules for obvious reasons. I don't know much about Inline Styles but as far as I know, you'd need to extract to render on the server.

JavaScript vs. CSS

Still, if you need to perform dynamic transforms or something like that you can always add IS. CSS is for styling... and if you need variables, mixins, etc, you can use SASS on top of CSS.

Autoprefixing

Prefixing in CSS is so easy, just add an autoprefixer or something. With inline styles you can do it with libs too but it's not providing anything else in my opinion.

Summary

With CSS Modules your style is separated, allows the browser to cache styles and server side rendering works. Inline styles can be used along with CSS modules so its main advantage is not so important. Also, it's not a so usual scenario.

Then, there is the theming issue... Theming with inline styles is difficult since inline styles have too hight priority and can't be easily overridden. It's true that there is no theming solution for CSS Modules either but you can use SASS loader and override SASS variables to customize components (see the approach in toolbox-loader). You can unveil structure in the component or provide custom classes to add your custom styles too. There are a lot of ways to solve the issue actually.

Finally, the developing experience is poor since you browse the DOM and see nothing but CSS mixed with DOM nodes which makes difficult to identify your structures and debug. And also I want to point that CSS Modules are, of course, not a perfect solution either. You are forced to use webpack or browserify... but is it an issue??

Of course this is sooo opinionated. I understand why people support Inline Styles but at the moment I'm more a CSS Modules guy :)

Thanks for asking for my opinion @oliviertassinari !!

robinweser commented 8 years ago

Wow. Great answer as well! Quite interesting to learn what people voting for CSS Modules think about it. I can agree to most of those aspects and totally understand why people choose CSS Modules over Inline Styles. I really have to try it some day..

Just one thing to add: To do server-side rendering you'll basically only need to transform an initial set of media queries until the DOM mounts. Everything else should just work fine. (Also prefixing with JS has one advantage: You only get what you need, no unnecessary prefixes at all but thats nothing that really matters.)

I think in the end it basically depends on the liking of each individual user which one to prefer. Seems like you can achieve quite everything with both solutions.

javivelasco commented 8 years ago

I agree with you @rofrischmann. As everybody says there are no better or worse solutions, just the most appropriated for your use case :)

ianobermiller commented 8 years ago

What about something like react-themeable? You'd still have to decide what to use for the default theme, but it would allow users to override the theme with whatever method they desire.

FWIW, Radium does have server side support for prefixing (via inline-style-prefixer), and keyframes and media queries are in the works.

jzelenka commented 8 years ago

CSS Modules with LESS/SASS or PostCSS chain solves most of problems with CSS mentioned in CSS in JS presentation.

From library user perspective, sometimes it is necessary to bend the look of the compoment in a way that library authors could not percieved. And it is much easier to edit some stylesheets than to fork or try to PR component.

With power of JS goes also responsibility, with inline styles it is possible to engineer logic into styling that is difficult to grasp for outsider. CSS rules and logic is at least well known albeit limited.

Inline styles clutter component source, e.g. in TextField 8 out of 23 props are styles definitions with proportional part of component source.

So I hope that you decide for CSS Modules and this awasome library will be even more awasome.

newoga commented 8 years ago

Since this project is currently using inline styles, I think we should stick with inline styles (or more importantly continue defining styles in javascript instead of CSS). That being said, I still think there is value in investigating approaches or styling libraries that support extracting inline styles into CSS. Right now, I think jsxstyle is the first one I'd like to try out.

Could it make sense to create a todomvc like project or branch that compares different approaches to styling for some of material-ui's basic components? We could use it as a basis for discussion and maybe we'll learn something.

I get a feeling that if this project's goal is really to support both react and react-native, there may not be a single approach that works and we may need to investigate how we can apply style transformations at build time.

neverfox commented 8 years ago

I think that idea. But what ever is chosen, one of the things that will be important to consider is making some of the more complex components more transparent to styling all the way down the internal chain of components, to @jzelenka's point about bending "the look of the compoment in a way that library authors could not percieved". I've run into that with the current incarnation, e.g. AppBar, where the style prop can only affect the root element. This is also a good opportunity to circle back to the Material Design document because some of the components don't currently follow some of the more explicit specifications all the way, especially under responsive conditions.

mgcrea commented 8 years ago

Not that it is worth anything, but from a React beginner's perspective (coming from Angular), and eager to use material design, the use of inline styles highly discourages me from using this library. I'm not willing to turn down so many useful features of CSS/LESS/SCSS and the ability to easily debug my styles only to gain minor portability improvements. I'm already transpiling with babel, bundling with webpack, so handling "native" styles comes at zero extra cost.

neverfox commented 8 years ago

@mgcrea I understand the concern, because I had them myself. But over time, I came to the conclusion that I had been missing out by not using JS styling. I can be more dynamic than I have been able to be before. One of the big laments is supposedly media queries, but the only time I felt I really had knocked it out of the park in terms of responsiveness was when I combined a listener to the viewport and dynamically altered my React components in JS in response. I'm curious what you think you'll br turning down or missing out on most, e.g. in debugging.

oliviertassinari commented 8 years ago

@mgcrea @neverfox Thanks for your point.

Where we are now

Regarding our current style approach, that's far from being ideal. We have to improve it for various reasons:

Where we can go

Radium or react-look are a good candidate. But I'm having second though. I realized by looking at source code of Radium: https://github.com/FormidableLabs/radium/blob/master/src/resolve-styles.js and react-look, if I'm not wrong, that they are travelling the all children tree. That's scarring me. I have no idea what are the performances implication of this.

... more importantly continue defining styles in javascript instead of CSS

@newoga I agree. I think that you hit the key point. I don't really care if it using CSS or inline style behind the scene. If we want to share some style at some point with react-native, we need to have it in js. Then comes the question of the implementation: There are Radium and react-look that use the inline style approach. I see some drawbacks:

I see some advantages (among the other stated before):

ianobermiller commented 8 years ago

@oliviertassinari you are correct that both Radium and React Look traverse rendered elements in order to resolve styles, and there can be performance implications. In fact, the React core team would prefer that we figure out a better way to hook into the cycle, but that would require making nontrivial changes to React itself.

If you use components well, keeping them small, and implement shouldComponentUpdate when needed, you will avoid unneeded render cycles anyway, which is the only time you pay the cost of traversal.

I would love a better solution, but I haven't yet taken the time to investigate adding such a hook to core.

wmertens commented 8 years ago

In a way, css classes are a space-efficient way to describe individual element styles, which is not available in react-native.

The proper solution then is to have an optimization pass as part of transpilation that converts inline styles to static and dynamic parts, and replaces the static parts with classes.

Non-trivial to implement, though :-)

On Sun, Jan 3, 2016, 9:48 PM Ian Obermiller notifications@github.com wrote:

@oliviertassinari https://github.com/oliviertassinari you are correct that both Radium and React Look traverse rendered elements in order to resolve styles, and there can be performance implications. In fact, the React core team would prefer that we figure out a better way to hook into the cycle, but that would require making nontrivial changes to React itself.

If you use components well, keeping them small, and implement shouldComponentUpdate when needed, you will avoid unneeded render cycles anyway, which is the only time you pay the cost of traversal.

I would love a better solution, but I haven't yet taken the time to investigate adding such a hook to core.

— Reply to this email directly or view it on GitHub https://github.com/callemall/material-ui/issues/684#issuecomment-168539863 .

Wout. (typed on mobile, excuse terseness)

robinweser commented 8 years ago

Right now I am focusing on DX tools such as a inline-style-linter with browser compatibility checking, etc..., but as soon as I am done with the an initial satisfying set of plugins, I will provide a proof-of-concept for some perfomance optimizations I experimentally played with some time ago (and then stopped, as so far I did not encounter "real" problems even with a large, UI-heavy hybrid mobile application).

I think it is not even too complex splitting styles if you follow some simple rules and covers up to 90% of the styles in an average project.

If possible I will provide an use-case example fro the hybrid app I was working on. e.g. a huge List with lots of inner flexbox <div>s, icons and different style on every ListItem.

I would actually love to have a proper solution to the performance issues using inline styles.

ianobermiller commented 8 years ago

I would actually love to have a proper solution to the performance issues using inline styles.

@rofrischmann I would love to have some evidence that there are actually performance issues to using inline styles, not just with specific techniques like traversing rendered elements.

STRML commented 8 years ago

I agree that there is little evidence showing that inline styles have performance implications in the browser itself, versus classes.

However they do appear to have a very real runtime penalty inside React due to all the object merging on every render.

For example, this is me simply typing into a Formsy-MUI text field, in a form with about 12 elements. It re-renders on change.

screen shot 2016-01-10 at 7 25 05 pm

The majority of the time in the profile was taken up on:

  1. GC from all the objects created and destroyed
  2. shallowEqual checks inside React to check for mutated style; this doesn't run in production but slows down dev. Mutating style was deprecated in 0.14 so there are expensive checks to warn for it.
  3. prepareStyles shows nearly 70ms self and 600ms total time. A lot of this appears to be due to a deopt due to misuse of arguments. I'll submit a PR for this.

In general, render() methods within MUI components completely dominate the tree.

In addition, inline styles:

  1. Are difficult to override, which leads to us having to be glued to the documentation to guess exactly which prefixed style property will actually allow us to modify styles or fix a style bug,
  2. Make looking at the DOM messy,
  3. Make it difficult to tell what the role of each element is, which is usually what class names do; this means I have to spend more time synchronizing the rendered output on one side and the inspector on the other, while munging styles over and over to get what I want.

As much as I liked how easy it was to just drop MUI components into my project and not worry, after much more use with MUI I'm convinced it was not the right decision. I am sure we can work out a way to make this work well with CSS Modules and vastly simplify the project and improve its render performance.

alitaheri commented 8 years ago

@STRML We are planing to utilize memoization to avoid this amount of merge and prepare calls. Thanks for benchmarking this. It surely shows how big a deal this is. Hopefully we'll do a 0.15.0 sooner so we can remove like 2000-3000 lines of useless code. This gives us much more flexibility and code quality to work with.

dverbru commented 8 years ago

Hi. Regarding vendor prefixes, I am surprised that nobody mentioned yet that doing browser detection based on the user agent string is a very well known bad practice. In my opinion, delivering all the prefixes in CSS to let the client use whatever works for it is actually the best option (and please note that bandwidth is not an issue after gzipping). Just my two cents.

STRML commented 8 years ago

I agree with that 100% @dverbru. To add insult to injury, material-ui depends on global.navigator, which has major problems with async server-side rendering.

STRML commented 8 years ago

Let me give another reason why inline styles have to go: component state.

For example, <Snackbar> has an open state.

It was designed only to open from the bottom, but in a project I wanted it to open from the top. With CSS, this would have been simple. With inline styles, it's not. Any styles passed to the component override all styles, so sending a new transform would have overridden both the closed and open state. I ended up having to do this:

import React from 'react';
import { Snackbar, Styles } from 'material-ui';

export default class FlashSnackbar extends ReactComponent {

  componentDidUpdate() {
    monkeyPatchSnackbar(this.refs.snackbar);
  }

  render() {
    return (
      <Snackbar
        ref="snackbar"
        style={{minHeight: 54, maxHeight: 96, display: 'flex', top: -4, bottom: 'auto', height: 'auto'}}
        bodyStyle={{boxShadow: '0px 0px 0px 6px rgba(0,0,0,0.4)', background: Styles.Colors.amber100,
                    lineHeight: '22px', minHeight: 30, maxHeight: 90, height: 'auto', padding: '10px 24px 6px',
                    textAlign: 'center'}
        ... />
      />
    );
  }
}

// This is a really good argument for not using inline styles
function monkeyPatchSnackbar(bar) {
  let getStyles = bar.getStyles;
  bar.getStyles = function() {
    let styles = getStyles.apply(this, arguments);
    const {
      desktopSubheaderHeight,
    } = this.constructor.getRelevantContextKeys(this.state.muiTheme);
    styles.root.transform = 'translate3d(0, ' + (-1 * desktopSubheaderHeight) + 'px, 0)';
    return styles;
  };
}

(for whatever reason, overwriting Snackbar.prototype.getStyles() alone didn't work, it was rewritten somewhere).

There's no way to overwrite vital parts of component styles without looking into the source.

terrisgit commented 8 years ago

So react-free-style is out of the question?

newoga commented 8 years ago

@terrisgit Nothing is really out of the question and a decision hasn't been made. We know there are issues/challenges with the current implementation and we are working on refactoring the codebase to get to point where we can experiment with alternative style approaches/libraries.

If you have any issues not already described above or anything you else you think that should be considered in the decision process, feel free to leave a comment and we will probably end up revisiting this whole topic when we're closer to ready to making a change.

newoga commented 8 years ago

@STRML Thanks for your input. It's really appreciated! :smile:

newoga commented 8 years ago

@dverbru Thanks for your input as well! :smile:

STRML commented 8 years ago

I think the best way for us to evaluate this is to generate a list of requirements we have for this solution, then choose which library fits it best. Qualitative things like the size of the community / uptake help, but in the end we just should be doing what fits this library best.

Here is a great list of CSS in JS Libraries for React.

I'll edit this comment if there are good contributions below.

  1. Performant styles definitions with minimal boilerplate.
  2. Extraction of styles to a <style> element or CSS file with readable classNames or importable references (so they can be overridden by library consumers). This is essential to prevent React from diffing a bunch of style properties as outlined by Pete Hunt.
    • Important thing to note here is that extraction to a CSS file is not what we want, as consumers must then import all styles (or one stylesheet per component). At that point, vanilla Sass sure seems like a lot less hassle. Therefore,
    • It should extract to a <style> element in the browser and support doing so on the server as well.
  3. Styles should be generated and rendered only when used (otherwise, as above, let's just use Sass)
  4. Styles should be deduplicated; using two <RaisedButtons>s should not generate two <style> elements or classes.
  5. Server rendering support.
  6. Theming support; we should be able to define variables in the styles that can be overridden by muiTheme. react-themeable can get us there and is compatible with most solutions.
  7. Usage should not require the consumer to execute an additional build step.
  8. Native media query support.
  9. Native :hover/:active support.

What I consider less important, but up for debate:

  1. Native :before/:after support.
  2. Prefixing support; IMO it's fine if it just generates all prefixes, otherwise prefixing via context.navigator is much preferable to the current approach.

Of the items in the comparison, I think the top contenders are:

Note I am not including Radium as it does not support extraction to CSS. IMO this is a step backwards and we've already seen what it feels like.

I also have not looked through all of the list. There may be some I have missed.

It appears that JSXStyle is the most fleshed out (or at least, it has the best README). Perhaps we should try converting a few components in each to really make a decision.

STRML commented 8 years ago

@dverbru Now that I think of it, do you have any references for that, in particular with reference to vendor prefixes (not platform features)? In general, doing assuming feature compatibility via user-agent is indeed a very bad thing. But trimming prefixed properties is generally pretty foolproof AFAIK. Famous last words I guess.

IMO I think we could drop the requirement wholesale and just use autoprefixer. It has great support and I agree that with gzip, the impact is negligible.

newoga commented 8 years ago

This package seems like it could be used for react inline styles specifically: https://github.com/postcss/postcss-js

I don't see why we couldn't implement that sooner if this is indeed the better approach. @STRML or @dverbru feel free to make a new issue on that particular point.

STRML commented 8 years ago

Nice link @newoga. I agree that this would be worthwhile while we figure out the other issues. Porting is labor-intensive, however, so of course it depends on volunteer availability.

ianobermiller commented 8 years ago

Some responses to @STRML:

material-ui depends on global.navigator, which has major problems with async server-side rendering

The particular implementation of using global.navigator may be poor, but using the userAgent to do prefixing is perfectly valid, and works just fine in a server rendered context, even if overlapping requests.

Let me give another reason why inline styles have to go: component state.

Your example has nothing to do with component state, and is a poor way to implement styles anyway. Ideally, the defaults should be some module level vars that cannot be overwritten at all. IMO this is a feature and not a limitation of inline styles; you can't muck with stuff that wasn't intended to be mucked with. For a library like material-ui, all extension points, including overriding styles, should be carefully considered to reduce the burden of breaking legitimate users with trivial changes. For example, if you are relying on some specific element hierarchy to do your styling via CSS, and they add in a wrapper div, your styles may break.

it does not support extraction to CSS ... this is a step backwards and we've already seen what it feels like.

It's not really as bad as it sounds, particularly with rich editors (for instance, there is a nice Atom extension for incrementing values, e.g. 5px to 6px, with a Chrome-like key combo) and hot code reloading. Once your inline style library commits to extracting to CSS, you have to solve the precedence issue, particularly when composing components and using them in slightly different ways from which they were intended; the move to inline styles was motivated by some real issues with CSS.


@newoga Honestly there is a lot of FUD in this thread, and I don't think the discussion has been super fruitful. I'd suggest locking it and just making the decision. Also I'm glad that Radium sparked your interest, but since the topic of inline styles is clearly bigger than Radium, you might want to change the title of this issue as well. Good luck with the choice, and I'd be happy to help evaluate the options (I did collate that gigantic table, after all ;).

STRML commented 8 years ago

@ianobermiller Using global.navigator does not work fine with overlapping requests. It would work fine via context. In any case, prefixing is the last of our problems.

Re: the state example, perhaps the component's source would make the problem more clear. Since we can't simply add a className to the component when it's open, specific styles must be added. In this case, the solution from the library's point of view is to add yet another injection point (rootOpenedStyle), but it's getting unwieldy. How many of these should we have (they're starting to dominate the API), and how many have been missed?

Every element that has children needs to define extension points for each of its children, and for every possible state (opened, closed, hovered, etc.). The combinations will dominate every element's API. This is a solved problem in CSS and incredibly difficult here.

This is not a feature, being able to do something trivial like change the height or position of a component should not be this difficult. The developers can and should not be expected to anticipate every way that a user might want to use a component.

Re: element hierarchy, I agree and believe this is even more of a reason to move to classNames. Otherwise, if the element you want to style has not defined every possible extension point, you need to rely on its internal structure to style it. Then, the library changes and your style breaks. That's why you use classNames via CSS modules or some other mechanism that guarantees non-clashing identifiers and support of :hover et al.

neverfox commented 8 years ago

how many have been missed?

More than a few.

Every element that has children needs to define extension points for each of its children, and for every possible state (opened, closed, hovered, etc.). The combinations will dominate every element's API.

This (the AppBar and IconButton are a good examples). I don't know if CSS Modules are the answer or not, but this is something to take seriously in whatever decision is made. One option (though I don't know how feasible this is) is to go heavily in the direction of avoiding overly nested uber-components. Make everything explicitly nestable and leave the building of complex components to the user (more like the approach of List and ListItem, for example).

ianobermiller commented 8 years ago

@STRML

Using global.navigator does not work fine with overlapping requests. It would work fine via context

Yes, I said that; the problem is the implementation, not the concept.

but it's getting unwieldy. How many of these should we have (they're starting to dominate the API), and how many have been missed?

Why is it worse to have a bunch of props instead of a bunch of CSS classes? I'd expect that an extensible component library has a significant number of extension points. A lighter-weight option would be to take in a styles prop, the contents of which are merged into the result of getStyles. That would give you basically the same thing as a bunch of global CSS classes, except you could actually type check it, statically or at runtime. Just because the complexity is split into two files doesn't mean isn't the same amount of complexity; the CSS will always be tightly coupled to the implementation.

Otherwise, if the element you want to style has not defined every possible extension point, you need to rely on its internal structure to style it.

You will inevitably end up with CSS classes that are added to the component even though the default styles don't actually use them.

Extensibility should be a conscious choice by library designers, not the default, as is the case with global CSS classes. But perhaps for a library like material-ui, the burden of doing so is too high, even with open source contributions. In which case, this certainly could be a plus of CSS.

Another possibility is that the base components be agnostic to style; they offer an extension point for just about everything, and you could pass in either inline styles or a classname (or any props, I guess). I'm sure we could come up with enough abstraction to make writing such a component tolerable.

alitaheri commented 8 years ago

@STRML All those issues are perfectly valid. We plan to promote composibility which will solve many customization issues. Plus we wish to add style and className to EVERY component.

It would work fine via context.

It is planned :grin:

Yes, I said that; the problem is the implementation, not the concept.

I completely agree with you. our current implementation is very bad. granted. But it can be improved.

My own opinion: Polluting global scopes (css classes) is a framework smell. Many things such as complex css queries can go wrong! I believe inline-style with user-agent aware prefixing is the future. Sure some implementations are weak for the moment, but atleast we will avoid framework smells, for example Table had some classnames and some people had issues with their css selectors! I mean just 4 classes caused some issues with some people. prefixing classes is not a good idea as some dynamic selectors will still pick them up. global scope is a bad idea!

this discussion is going off topic. All these issues mentioned can be solved in both css and inline style. I'm all for inline-style.

I think we should come to a conclusion on CSS vs Inline-Style first and then see what library/framework/method/concept to use or follow in that direction. Just as a reminder, this library used to work with css but then it migrated to inline-styles for what it's worth take a look at #30.

@oliviertassinari @newoga @STRML @ianobermiller @neverfox @mbrookes And everyone else.

Lets please discuss this before deciding on library. After the decision is made. I will lock this issue and open a new one for further discussions. Thank you everyone.

oliviertassinari commented 8 years ago

IMHO it would be far better to adopt a solution, where we write the style in JS. Hence, I would just drop CSS Modules.

this discussion is going off topic.

Yes, can we stick to radium in this thread and move to https://github.com/callemall/material-ui/issues/1951 for the other style related discussion. Thanks.