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

Use of media queries, pseudo elements, hover, class names on inner elements #1951

Closed
adamscybot opened this issue Oct 21, 2015 · 40 comments
Closed

Comments

@adamscybot
Copy link

Since the change to inline styles how am I supposed to:

  • Add media queries so I can change the styles per breakpoint of material ui components.
  • Add pseudo elements.
  • Add hover affects.
  • Override the styles of inner components.

All the above would be possible (albeit hacky) if every component produced elements with classes set (using !important in my CSS). At the moment it creates elements with inline styles but no class, so they cant be targeted and overridden in CSS.

Since the change, this project is extremely difficult to work with if you are building a responsive site with any degree of customisation on top of the stock components. I have since given up on it and moved to something else as it is a constant battle with this library to display elements how you want them to. Beforehand, we could just use CSS as everyone has for years (decades?).

@JEGardner
Copy link

+1 Inline style implementation breaks how most devs want to build sites.

@adamscybot
Copy link
Author

At the moment, I'm having to do things like this:

.new-booking > div > div:first-child > div:first-child > div:first-child > div:first-of-type {
    overflow: visible !important;
}

@cosminnicula
Copy link

@adamscybot for media queries and pseudo-classes you can use Radium (http://projects.formidablelabs.com/radium -> check out the API Docs). For example, with Radium you can have things like a media query @media (min-width: 992px) or a pseudo-classes :hover right in your inline style - which is super useful.

Pseudo-elements are nothing more than JavaScript variables and math -> check out this cheatsheet https://youtu.be/ERB1TJBn32c?t=17m49s
cheatsheet

For "overriding the styles of inner components" I guess you're referring to CSS descendant and child selectors. For this I don't have an answer...unless an external class written in old-fashion CSS is considered a good answer. Maybe someone could clarify what is the appropriate way of achieving this with inline styles.

Cc @chantastic, @jquense

@jquense

This comment has been minimized.

@cosminnicula
Copy link

@jquense maybe you can help us a bit. Here react-bootstrap/react-bootstrap#362 you mention that descendant and sibling selectors are hard with inline styling. Also the React Widgets as well as React Bootstrap libraries heavily relies on external CSS classes rather than inline styling. I was wondering if you could share some code with how hard it is to achieve descendant or child selectors with inline styling, given the fact that media queries, pseudo-classes and pseudo-elements can be in a way or another elegantly solved.

@adamscybot
Copy link
Author

Regarding selecting inner elements -- old fashioned CSS would be an OK solution to me. However, as mentioned, its very difficult to reliably do this without lots of:

.new-booking > div > div:first-child > div:first-child > div:first-child > div:first-of-type {
    overflow: visible !important;
}

This works but is obviously a bad idea as any updates in material UI that change the DOM layout will break these selectors. This would be fixed if every component in material-ui was given a classname, including inner ones. Is this something that would be accepted in a pull request?

It's a real shame that this project did its own implementation of CSS for components. In general, I disagree with the idea completely, but if this is inevitably going to be done an off the shelf solution should of been picked such as radium which is more feature-full. Though I don't know how mature those solutions were when the decision was made.

@cosminnicula
Copy link

@adamscybot agree on the fact that relying on those kind of selectors is fragile because they're tightly coupled to the HTML structure and also not opened for further customization because of !important.

However, I don't see why Material UI won't accept pull requests for including customization points (CSS classNames or inline styling parameters or whatever) for every inner component. jQuery UI has customization points (CSS classes) for all its inner components. Take a look at the jQuery UI Selectmenu widget below:

1

It would be naive to think that a component library like Material UI is flexible enough to fit everyone's needs by only providing top level styling customization points and ignore the customization points for inner components.

Including @hai-cea in the loop for helping with this problem

@adamscybot
Copy link
Author

Sometimes, even if you want to keep the components the same and just want to change the styles of the root component -- you can't do things you'd expect to be able to. For example, if you change the width and height of FloatingActionButton, it doesn't alter the width and height on the inner components meaning the ripple and hover affects look wrong. This is actually because some components have bad CSS (using hardcoded magic numbers instead of percentages). However, it's very irritating to have to submit an issue/pull request every time this happens rather than just fixing it ad hoc at the time by using inner selectors.

@chantastic
Copy link

I've been pinged on various networks to come weigh in on this.

Breakpoint, hover, and pseudos can be pretty tricky in inline-style systems. As mentioned above Radium is a really nice abstraction that feels very Sass-like.

I wouldn't recommend going it alone, it's a tricky problem.

I can't really speak to the rest. I am not an active part of the material-ui community. It would be disrespectful voice an opinion on any of the other matters raised here.

I saw that you're using that slide as a pseudo-class cheatsheet. I have this codified in a repo, if that provides an help: https://github.com/chantastic/pseudo-class

Cheers!

Chan

@vjeux
Copy link

vjeux commented Oct 31, 2015

The way we deal with this on React Native which uses inline styles is by providing "customization points" as @cosminnicula mentioned. Using the same example, it would look like this:

<SelectMenu menuStyle={{paddingRight: -5}} menuItemStyle={{margin: 10}} />

and to implement it

var SelectMenu = React.createClass({
  render: function() {
    return (
      <div>
        <ul style={[styles.menu, this.props.menuStyle]}>
          {this.props.items.map(item => <li style={[styles.item, this.props.itemStyle]} />)}
        </ul>
      </div>
    );
  }
});

@cosminnicula
Copy link

@hai-cea, @oliviertassinari any thoughts on this?

@oliviertassinari
Copy link
Member

Those is are good questions!

Override the styles of inner components

The CSS approach we had before was adding an extra layer of API (className). And was used by people to customized component. IMO it wasn't great since it was making our implementation detail a global API by default and likely to break.
With the inline style approach, you have to use specific style properties for inner components.
If you need a property that doesn't exist, we will happy to accept pull request. (as proposed @vjeux with customization points)

Add media queries so I can change the styles per breakpoint of material ui components.
Add pseudo elements.
Add hover effects.

That's the most annoying part after our migration to the inline style approach.
A better long-term solution would be to replace our custom solution with something like Look (or Radium once this issue is resolved).

However, it's very irritating to have to submit an issue/pull request every time this happens rather than just fixing it ad hoc at the time by using inner selectors.

@adamscybot You are probably not the only one how faces this type of issue. It would be great contributions 👍.

@Dattaya
Copy link

Dattaya commented Nov 12, 2015

Radium has resolved that issue and released version 0.15. 😋

@mbrookes
Copy link
Member

mbrookes commented Jan 7, 2016

This looks promising https://github.com/rofrischmann/react-look, although I haven't dug into the details.

@oliviertassinari
Copy link
Member

@mbrookes I was thinking about using Radium. That's more bulletproof.

@mbrookes
Copy link
Member

mbrookes commented Jan 7, 2016

Oh yes, I see you Look'ed (groan) at it already.

@jzelenka
Copy link

Radium is using onMouseEnter / onMouseLeave events and state for tracking state for :hover.

It is slower than css :hover.

On complex page with many elements, slower machine and quick mouse movement it can even miss some events. So your element stays 'hovered' even after mouse leave it.

For hover, any inline styles solution is worse than css :hover.

@STRML
Copy link
Contributor

STRML commented Jan 12, 2016

With more development in an application that uses material-ui, I am starting to think that inline styles are a fool's errand in a library. I greatly enjoy CSS Modules in an application, but they're not usable out-of-the-box in a library. At this point in time, a library should simply use namespaced classes.

With proper css class names, developing with material-ui would be a breeze. Sure, it would be possible (and likely) that you'd include more CSS than you need, bloating your build. But Radium is a 50kb minified (114k unminified) library. This means that importing even the smallest component would add 50kb+ to your build. With CSS, at least you have the ability to cut dependencies you don't need, or offer 'material-ui/styles/menus/menu-item.css'-style imports for Webpack users.

Regardless, web developers are already set up to handle dependencies with css. If we namespaced all the classes it would work fine. Yeah, it would pollute the global namespace.

Is it ideal? No. But I don't think the ecosystem is there yet to handle modular CSS. <style scoped> will help a lot in the future, allowing us to choose predictable classNames for easy extension (without needing JS to lookup the generated names) while avoiding global pollution. But it's just not there yet.

As mentioned in many comments and threads, including this one and #684, inline styles just have too many compromises.

@dlebedynskyi
Copy link

@STRML have you looked at https://github.com/gajus/react-css-modules? it is pretty much css modules for webpack and browserify. Their for you can have same size optimizations on build - since you can reference only what you need. One thing I do not like for radium is that in the end it is still pure inline styles that are horrible to read in dev tools

@STRML
Copy link
Contributor

STRML commented Feb 2, 2016

@dlebedynskyi Yes, issue is that as a library, you can't rely on all consumers using webpack/browserify. So CSS Modules is more or less out of the question - there's not really a good solution to this that I'm aware of, for all packaging possibilities.

@dlebedynskyi
Copy link

@STRML I would assume that webpack/browserify will actually make a bundle for you. And I would expect css to be bundled as well there. And at this point you could create whatever bundle you want - as a whole lib or as umd module per each component, if we are talking about lib deliverable.

As for case where this package is actually used in development of something bigger then hello world I would expect browserify/webpack/system-js builder anyway.
Of course it can be handy to have something like https://github.com/ludohenin/gulp-inline-ng2-template for this purpose.
IMHO, using anything that in the end will be producing inline styles is very bad idea. It will still be unreadable and hard to customize/override.

@oliviertassinari
Copy link
Member

It will still be unreadable

I would say it's more an issue with your debugger tools than with the approach itself.

and hard to customize/override.

I'm wondering if it would help to append a property in development so people know which dom element a style property is affecting. In the same way, a className is a public API that helps to identify dom element with the dev tools.

@STRML
Copy link
Contributor

STRML commented Feb 3, 2016

I would say it's more an issue with your debugger tools than with the approach itself.

If a library does not work well with every inspector in every browser, is it a problem with the inspector, or the library?

I'm wondering if it would help to append a property in development so people know which dom element a style property is affecting. In the same way, a className is a public API that helps to identify dom element with the dev tools.

That would be nice, but it shouldn't be just in development, because users will write CSS selectors to target those elements and will be surprised to see them break in production. There should simply be namespaced classNames on these elements.

@nikgraf
Copy link
Contributor

nikgraf commented Feb 21, 2016

I was doubling down in inline-styles for a while, but the main issue with inline-styles is that you can't do media-queries in combination with server-side rendering. Radium is really smart about it, by injecting a real style tag. Still I'm not sure if this is a ideal solution. To me className based systems like CSS Modules or JSS feel like a cleaner solution since they still allow media queries & pseudo elements out of the box.

I like @vjeux's suggestion regarding "customization points". We did that in Belle, but we are considering to move to a single theme prop that contains all the styles. This significantly reduces the amount of props and you can prepare the theme in your code before without polluting your JSX. Could be nice for MaterialUI as well?!?
In addition I wonder if merging or overwriting styles is a better solution when providing data to a "customization point".

@nathanmarks
Copy link
Member

@nikgraf That's my one concern about inline styling for production -- scalable inline styling with support for media queries requires significant a engineering investment just to keep performance reasonable.

@STRML having namespaced classnames also aids in writing E2E/acceptance tests where react/vue/angular/whatever is irrelevant -- you just want to be able to easily select the critical elements which is hard when you essentially have to duck type your selectors with the absence of classes or other identifiers.

@STRML
Copy link
Contributor

STRML commented Feb 28, 2016

It certainly feels like a massive compromise just to eliminate a css
dependency, especially when so many toolchains are using less/sass. I'm
not saying there's a better solution than less/sass or simply delivering
a full css file (or a common/component structure), but it's certainly
better than this.

Nathan wrote:

@nikgraf https://github.com/nikgraf That's my one concern about
inline styling for production -- scalable inline styling with support
for media queries requires significant a engineering investment just
to keep performance reasonable.

@STRML https://github.com/STRML having namespaced classnames also
aids in writing E2E/acceptance tests where react/vue/angular/whatever
is irrelevant -- you just want to be able to easily select the
critical elements which is hard when you essentially have to duck type
your selectors with the absence of classes or other identifiers.


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

@mbrookes
Copy link
Member

@nikgraf:

we are considering to move to a single theme prop that contains all the styles. This significantly reduces the amount of props and you can prepare the theme in your code before without polluting your JSX. Could be nice for MaterialUI as well?!?

We discussed it here #3130, but there were performance concerns.

@nikgraf
Copy link
Contributor

nikgraf commented Feb 29, 2016

@mbrookes I agree that a level of nesting means a bit more effort in shouldComponent update. We should be able to create helper function which checks the theme and then it shouldn't be more than accessing a prop one level deeper and to the same amount of props checking. Unfortunately I don't have numbers on the level of impact. Anyone tested this so far?

Re classNames: on the React Conf I asked in the Q&A what they think the future of styling will be. ClassName based or inline styles? Sebastian Markbåge mentioned he is opinionated about it and thinks it will be inline-styles.
To give some context: the React team spoke a bit about how React in the future might be able to take care of layout on the Web like with React Native on iOS/Android. Right now the API's are missing in the browsers, but maybe sometime in the future this might be possible and a styling system in JS might be the way to style components.

@mbrookes
Copy link
Member

I put together a quick test, to show the reduced API, but didn't optimise in shouldComponent, or test for performance impact. Not sure I have the code around any more though, such as it was.

@ffxsam
Copy link
Contributor

ffxsam commented Mar 4, 2016

@oliviertassinari:

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

Could you elaborate on why you think inline JS styles is a better approach than CSS Modules? Obviously CSS in React is all over the place these days, and I'm trying to get a sense of what I'd like to use in my projects going forward.

@oliviertassinari
Copy link
Member

@ffxsam You get me wrong, I don't think that inline JS styles is a better approach than CSS Modules. But I do think that writing the style in JS is more valuable.

We have already seen this pattern in the past with HTML and React.
IMHO the main advantage of React over any other framework, like Angular is that React in bringing the HTML to the JS and not the other way around.
I think that doing the same with the CSS is as valuable. For instance, I really like the styling approach used by react native.

However, the implementation isn't easy. He are intensively using the inline style approach here. We are using it enough to see that this approach isn't perfect and has big issues.
But, inline style isn't the only answer to the styles in JS. The implementation could be using a combination of CSS classes and inline style. Have a look at react-look. I'm really interested in this lib.

Regarding the future, as far as I know, Facebook see React as a platform and not a web library. They are really pushing forward to unify the web and the native environment, for instance they are considering using css-layout for react-dom. That could be another answer to how to implement styles in JS.

@codinronan
Copy link

Watching this topic with great interest. Hopefully the MUI folks decide on a solution, we love this approach and the library and are actively thinking about ways to help improve it.

@tintin1343
Copy link
Contributor

tintin1343 commented May 3, 2016

Closing this in favor of #4066

@jeffschwartz
Copy link

@STRML I fully agree that using namespaced css modules would be so much better than in-line styles; so many problems could have been avoided had that been chosen. I've used BEM with React with great success and I would love to see Material-UI maintainers rethink this issue and opt to rewrite the library using BEM or some other suitable namespaced approach. I rather like the UI that this lib provides but after cutting a branch of an app that I am currently developing with it I have decided to abandon the library due to its use of inline styles.

@STRML
Copy link
Contributor

STRML commented Aug 23, 2016

Yeah. This whole library has been hamstrung by this unconventional approach to styles. It's unfortunate but I'm hopeful a solution will emerge in #4066.

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 23, 2016

I can give a quick update regarding this issue.

  • @nathanmarks Has built a CSS-in-JS lib for our use case: stylishly. It's working great so far on the next branch.
  • We are investigating in using JSS instead to take advantage of a larger community. That requires some tradeoff.

@devarsh
Copy link
Contributor

devarsh commented Feb 2, 2017

Any thoughts on implementing the approach taken by react-toolbox

@oliviertassinari
Copy link
Member

@devarsh Could you elaborate on what you mean with approach. I'm not familiar with it. Thanks.

@breadadams
Copy link

breadadams commented Apr 30, 2018


**EDIT**: Damn, I feel stupid, I was doing `content: ''`, as opposed to `content: '""'` 🤦‍♂️ 

https://github.com/mui-org/material-ui/blob/b7ce7e5fa57c215bf759e5e31d1436adf212b5a2/packages/material-ui/src/Input/Input.js#L34-L35

@jeffersontpadua
Copy link

For anyone trying to use ::before selector, checkout @breadadams comment. Thats the right thing to do, and really easy it's really easy to miss.

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

No branches or pull requests