Closed nathanmarks closed 7 years ago
Some further questions:
6) Will we remove or aim to remove xxxxStyle
props and have users pass in an xxxClassName
for overriding styles? Or will these be complimentary?
7) Will we allow override of styles via CSS and simplify our components interfaces. So if I want to override menuItemStyle
in IconMenu, do I create a style = StyleSheet.create({ IconMenu: {MenuItem: { color:red }})
type override?
@chrismcv My personal opinion is that we definitely need to leverage the solution to remove these super specific props we're seeing proposed: [TextField] add floatingLabelFocusStyle prop #4043
Before we know it, people are going to be asking for props for subcomponent styles for every possible focus/active/hover/whatever state.
I see so many issues that are "how to style XXXX", "can't style XXX inside YYYY", "add somethingInsideToTheLeftButSlightlyMiddleFocusHoverActiveStyle
prop to XXX please"
@chrismcv in 6. regarding the more general xxxxStyle
props vs classNames, what do you think? with some of our components you're gonna need to be able to reach in one way or another (fortunately we'll be able to use :hover
etc though!).
@chrismcv I think that xxxxStyle
props should go in the style
property, not the className
-ified style object.
@nathanmarks Thanks for bootstrapping the conversation. That's definitely one of the major issues we need to address for the next releases π― .
Outside of performance issues
For me, that's one of the main concern I have with the solution we will choose. We don't have many benchmarks, be we can easy imagine that we lost many CPU cycles with our current approach. The CPU cycles are lost into memory allocations and garbage collection for our inline style object.
@oliviertassinari
I am playing around with a couple of the JS style solutions at the moment. It's interesting but clear that it is still "early days" so to speak.
One thing I was thinking -- outside of dynamic style properties (for eg, if the component has a color
prop), should we be changing the way we create the base style object?
It seems as though with the JS style libs, to get maximum performance you should use their StyleSheet.create()
method once and have keys (css "classes") in that object for props/states such as open={true}
rather than dynamically building the style object literal with a couple of conditional properties and passing it to the stylesheet factory method every render for every single style.
Even though the stylesheet selectors get cached in the sense that they are written only once to the DOM (and in some libs only written to the DOM when needed for the render()
), we're still wasting resources with some calculations + object creation + garbage collection as you mentioned above if we are creating a new style object each render just to have it thrown out.
Hmmm... I left a comment here yesterday (Sunday). Did it get deleted?
@rvbyron I remember it clearly. I certainly didn't delete it though.
@rvbyron I do remember it too. I have no idea what happened.
@rvbyron - had it in my email, so back here in quoted form!
Well, I for one would like to see a number of things take place.
A) Yes, by all means get rid of using the element's style parameter at the library level. All of the components should be defined using className. Having the components use style destroys all of the power CSS offers. B) It would be great to have classNames automatically attached to the root element of each component (e.g. The RaisedButton component would implicitly have className="mui-raised-button"on the component's root element). It would make styling a whole lot easier. That could even be configurable. C) Get themeing out of the muiTheme.js files altogether. A themeing muiTheme.SCSS file would be much better in that it would allow any properties chosen by the theme creator to be applied and not just the ones specifically allowed by the component. Of course this would probably require that B be implemented and active. D) React-look appears to be a good compromise as it converts JSON objects containing style properties into classNames. It makes things easier to override. However, as I said in C above, I would still like the themeing base to be created in SCSS. I am pretty open on which CSS assist package to use. But I would stay away from anything that wanted to use the element's style parameter over the className parameter.
@chrismcv Thanks dude!
I think that what @rvbyron has to say is important because in some ways, JS styling is very much a paradigm shift from regular CSS. Most people are not using JS styling in their day job and it requires a different way of thinking + makes it harder for designers who may usually contribute to projects that aren't so proficient in JS.
It's important to consider all the angles here.
@oliviertassinari @chrismcv
One thing I realised about react-look
is that you need to wrap all of your components in the look
HoC. This seems pretty invasive, it even hijacks the render()
function, stores super.render()
in a const
and performs style resolution operations that way. Some other solutions such as Khan/aphrodite just require a function wrapper around the styles.xxxx
reference from Stylesheet.create()
inside className={}
.
Hijacking the render()
function just for css seems over-engineered to me. It tells me that the React built in tooling/support for this sort of deeply integrated styles functionality is just not there, I'm not sure about the idea of a CSS helper HoC controlling the render for all of our components.
Thoughts?
Hi Material-UI team!
Have been following you for a while and now, seeing this topic, I also wanted to contribute with some thoughts why moving from inline-styles to css might be a good idea. I have read a lot of discussions in this repo that cover the rendering performance, styling children/nested elements, the media queries, pseudo-elements, etc., so I'll focus on something else that I haven't met discussed yet, but which I think is also relevant. And it is styles organization. Here is what I mean:
size={40}
sets width, height and line-height to 40px
)When it comes to adjust the styles there are at least 4 different locations (π ) to look at, which makes it hard to investigate issues.
When styles come with css you'd see the selector and know exactly where to fix. With inline-styles it takes time to understand where the particular property comes from, and where and what should be modified. And if developer modifies it in the wrong place (let's say in the styles
area instead of size
parameter, since nothing actually tells that size
is the style) it will affect the whole width=height=line-height=40px
block performance, or will lead to writing again width/height/line-height
which will make size
parameter inapplicable.
In addition it also makes it hard to investigate which of the parent elements have specific properties applied, because all you can see in the inspector is element.style
.
When styles come with selectors (class names) it gives much more control over the styles structure than inline-styles.
Update: Forgot to mention the suggestion (which this topic is about):
/component
--component.js
--component.css (or sass that is converted to css when the page is rendering)
This structure will keep component in the scope, but will give more control over styling. And a BEM className convention will help to avoid unnecessary nesting:
.mui-component {}
.mui-component__child {}
Which overall will help to implement, adjust and maintain Material-UI styles.
@chrismcv
Thanks for finding the email and placing it into the commentary!
What about a "behavior" concept of importing the styling technique that best suits your needs. An import of "StyleTheme" or "ClassTheme" could be used to select the behavior. StyleTheme could refer to the muiTheme object material-ui has today and make the component centric styling developers happy. Or, ClassTheme that would use an SCSS file that is built from the muiTheme.js object (synced) making the CSS centric developers happy. That would mean that all components would then need to accommodate {...theme.ComponentName} in the render elements.
To offer a very simplified example, a RoundButton component would have the render method like:
render() {
return (<button {...theme.RoundButton} />);
}
For the StyleTheme behavior, theme.RoundButton would contain:
{
style: {
display: 'inline-block';
borderRadius: '20px';
width: '40px';
height: '40px';
}
}
For the ClassTheme behavior, theme.RoundButton would contain:
{
className: 'mui-round-button'
}
A combination of the two could be retrieved as ClassStyleTheme behavior, theme.RoundButton would contain both:
{
className: 'mui-round-button',
style: {
display: 'inline-block';
borderRadius: '20px';
width: '40px';
height: '40px';
}
}
The mui-theme.scss file would be generated from the muiTheme.js file and reflect in SCSS the exact properties the JS file contains. The camel casing of the JS names would be converted to more standard class names:
mui-round-button {
display: inline-block;
border-radius: 20px;
width: 40px;
height: 40px;
}
Any CSS customization to the MUI components would simply be identical classes loaded after the mui-theme.scss file was loaded, thereby overriding the mui-theme properties.
@oliviertassinari
I think that using something like scss
(+ css modules for namespacing/hashing) has a lot of benefits. Is this something you are 100% opposed to? I'm probably slightly biased because it is what I am accustomed to using at work, but a lot of people are in the same boat.
My main problem with the JS styling situation is it feels like all of the solutions are still a work in progress. There are not a lot of real world performance evaluations out there to look at, the libs we are looking at are unproven and I feel as though we are spending a lot of our time trying to figure out how to do styles in JS properly. The main goal of this lib is to implement the material design spec and I can't help but feel that this is hamstringing us at the moment. (we def need to sort the perf impact issues)
I think we have different concerns regarding this.
All seem good when you're writing an application, no one outside your code needs to change anything.
But with libraries things are different, all the above concerns are there, plus the following:
What we have right now somehow handles all those concerns with these caveats:
somethingInsideToTheLeftButSlightlyMiddleFocusHoverActiveStyle
If we change our styling approach we'll have to do it all over again. If it comes to that we should be sure to answer all those before starting migration.
In my humble opinion, I think CSS and what ever that compiles to it are not parts of the future. I think we all agree that inline-styles made our lives a lot easier. Honestly, the only limitation I see with inline-styles is :hover
!
We can solve those issues with doing a bit of design.
ListItem
with a LOT of small functionalities embedded in it? we can break it down and make it composable, then we don't have to add monstrosities like somethingInsideToTheLeftButSlightlyMiddleFocusHoverActiveStyle
:hover
I think we can live with that until browsers or react does something about it.My point is, we already address a huge number of concerns with our current styling approach. It has limitations because we don't follow all the patterns around inline-styles. if we start breaking down our components and make them a lot more composable then we don't really have to worry about anything. take MeuItem
I took that out and made it composable, you see how easy it is to use and how performance can be improved for it using pure rendering!
So instead of changing our approach and solving all these issues again let's invest the time improving what we already have. we don't need to support media queries, we can change our components in a way that's easy for others to implement responsiveness upon them. Hard to debug? if we break down our components then the inlined styles will be smaller and easier to read. debatable though! I mean all you need is a break point to see what participates in Object.assign
to see where the styles are coming from.
That's of course my own opinion I'm very open to discussion.
@alitaheri
You raise some great points. Thanks. I'm wondering if some sort of hybrid solution is a possibility. Seems that no matter what angle we take there are a bunch of issues π
I have time to work on this next week -- let's have a chat and see if we can come up with a clever idea.
I agree with trying to minimize the amount of change and having to re-solve already solved issues. There's 2 important goals here in my mind:
Need to think about this more π but as long as we manage to solve those 2 points I'll be happy with our solution.
Another random note -- it would be great if we had an automated way to enforce certain code conventions and design patterns for inline/jsstyle/whatever across the lib.
Is this something you are 100% opposed to?
@nathanmarks I'm not 100% opposed to use SCSS (I'm using it at work). But from my point of view, it's a dead-end. One of the main advantages of React is to abstract away the rendering specificity of browsers: the DOM. As far as I know, the long term goal of the React Core Team is to provide an API to write cross-platform component. Actually, they are currently experimenting into this direction. SCSS won't help.
I completely agree with @alitaheri. We could start with those two steps:
getStyles
pattern everywhere so we can more easily migrate later.
Will we be able to use a codemod? Who knows.@oliviertassinari I agree with the dead-end conclusion -- it's not the future. I brought it up largely to stimulate the conversation about how to make JS styles work for us. I knew some people around here had some good ideas surrounding the issues π Wish FB were a tiny bit further along with their experiments!
I actually think the getStyles()
pattern we currently employ has it's own problems. @alitaheri and I had a pretty good chat today about what to do to improve our JS style setup (including theming!). I'll reply back tomorrow with some more info once I get a chance to jot down some notes!
Next week I'm on vacation and I'm going to experiment with a solution to our current problems while keeping all styles JS based.
Just read this post on Lessons Learned At React Amsterdam (https://medium.com/@kitze/lessons-learned-at-react-amsterdam-51f2006c4a59#.o12h794or) and one of presentations was about solutions for styling in React: http://slides.com/kof/javascript-style-sheets#/ A timely presentation for this discussion.
One requirement that I keep thinking about and that I haven't seen explicitly stated (at least that I recall) is the need to support web-only vs. web and react native. If the intent is web-only (i.e. react native isn't a requirement) then solutions that leverage what browsers already efficiently and robustly support and people are very familiar would be a good thing. If supporting react native is a must then that opens up a different set of needs & requirements. Just something to think about and keep in mind as the technology choices, compromises, etc. are evaluated.
@hburrows
Thanks for sharing that presentation!
@alitaheri
We may want to look at the lib mentioned in that presentation (here's a demo). It could save us a lot of work. I was looking at it previously but there were a few things I didn't like (such as the requirement of wrapping every single component in a proprietary HoC). However, there is some discussion about some of that happening here . He mentions that implementing changes like this would allow for HoC-free use. I do prefer that method/design for the sheet writing (also seen in aphrodite
).
Alternatively, we could always create our own react bindings for jss
that can work with mixout
.
The lib author also said this, which falls in line with my own thoughts on the matter π :
However you need to understand that not everything makes sense to be style sheets. For the most dynamic changes you should use inline styles.
@nathanmarks The implementation is rather very small and very easy to mixout :grin: https://github.com/jsstyles/react-jss/blob/master/src/index.js I think this library can help us out :+1:
@alitaheri Yeah I agree!
I'm experimenting with some custom bindings for jss
as well -- there's a couple of issues/limitations I want to try tackle. (One though may require work on the core lib)
There seems to be a prevailing sentiment in the React community that JavaScript styling is the best solution. That sentiment is fine, I am not in disagreement with those who have it. However, the way JavaScript styling is implemented is almost always without regard to the CSS developer. The implementation all too often favors style=/[element property]=
over className=
and prevents the CSS developer from doing their job.
As I offered in a previous comment, I think the two could live nicely together. One must simply take a behavioral approach to applying the style properties. The library must accommodate the direct application of style properties to an element, or it must allow the indirect approach of creating a className comprised of those style properties. A library that allows the developer to choose the behavior seems like an ideal solution.
There are many reasons CSS deveoper's don't want to simply ditch their approach. Search for "why use css" to see about why people find it beneficial. You will find abstraction of styles and a powerful selector language to be among its benefits. Let's not forget the enormous amount of code that exists today that benefits CSS users.
Again, this is not intended to be a pro CSS commentary by any means. There are those of us that would like to use CSS for styling. This comment is intended to encourage an architecturally neutral styling approach that empowers developers to have the choice.
@rvbyron One of the main reasons we want to change our current approach is to make it easier to customize the components using css too. using a library like jss
will allow us to use the css features while taking advantage of js-styles. And since it turns those styles into css, they can be easily overriden with an extra classname without the ugly !important
hack.
@alitaheri Great to hear! I am really looking forward to seeing material-ui accommodate a className based approach.
Again, fantastic to hear on the className approach. With this in mind, I have some questions on how it might be implemented:
@rvbyron
Ideally:
Customizable prefix. Dash notation of some sort for better dev experience and a hashed suffix which can be also be set to a fixed string (so won't change if the theme settings are changed) using a custom theme ID. Including a less verbose production option, or even a callback function to customize. And I'd assume a class on the root is usually always gonna be necessary anyways.
I think there are many requirements listed and suggestions offered. So, is there a take away from all of this discussion? What is the next course of action? Is there any work being done on implementing react-jss into this material-ui?
@rvbyron Yep, we're experimenting with a couple of rough ideas behind the scenes. Will update this issue when we've got some more concrete ideas.
In my humble opinion, I think CSS and what ever that compiles to it are not parts of the future. I think we all agree that inline-styles made our lives a lot easier. Honestly, the only limitation I see with inline-styles is :hover!
The issue is that :hover is a very basic, very vital part of styles. Futurism aside, it might be prudent to avoid premature commitment to a solution that must resort to cheats and work-arounds to reimplement a native feature.
@wtgtybhertgeghgtwtg we're working towards a solution that uses real stylesheets so functionality such as :hover
is supported.
Given that the major points have been decided (JSS styling via className and root element classNames), is there any roadmap and timeline for material-ui v0.16.0 release?
@rvbyron We don't have a promised timeline, but as of now, styling approach and performance is our primary focus for the v0.16.0 release and something we're actively working on now. We will likely use this issue in the near future for community feedback once we've settled on the right direction in a few other areas as it relates to internals, API, migration, etc.
@newoga Thanks Neil! I appreciate the update. As for the other issues, I would like to add that just going to a className based system can be a visually breaking change for many and I would suggest confining this release to just that. Other changes in addition to that might really impede a migration. However, I know your team will pick the best balance.
@newoga over here jss has been called out as the future of the styling system for material-ui. Is this a concrete enough plan that others could start onboarding jss into our projects now, in anticipation of the 16.0 release?
@sorahn not yet
@sorahn This issue is designed to get community feedback about what is still a really challenging problem to solve. No official decisions have been made. As of now, everyone should only be picking and using a style library or approach that they have assessed is best for them and their project.
For the record, the goal of this styling change isn't to migrate everyone to some different standard, but to make it easier to use and style Material-UI components regardless of what styling approach you use with it.
tl;dr No! Do what's best for you, not what anyone else says :)
@nathanmarks @newoga Thanks for the update!
We're running into the limitations of responsive design + server side rendering with all inline styles, so we're investigating the alternatives. No particularly strong opinions about (s)css-modules or csx or whatever, but we love material-ui so it'd be great to just follow along with whatever you guys are going to do :)
I ran into media queries, read this thread (and others). I dove into some of the potential solutions and articles.
I really like the JSS + CSX solution (from a developer joy perspective).
Similar to @sorahn, I'd be happy to adopt a material-ui standard, I hope @nathanmarks work in this direction validates this approach. Also, it seems that both the CSX/JSS developers are eager to help and promote adoption of their libraries.
@rosskevin Working on it as we speak -- there's things to consider here that haven't been covered in any of the solutions to date.
Our requirements are much broader than the existing solutions are capable of supporting because of design limitations or opinions. This is largely because the existing solutions have been developed with apps in mind where you have full control over the consumption of your component APIs versus a library where you have a diverse set of users wanting to style their components with different tools.
@sorahn Quick question -- what are you doing about vendor prefixes with server side rendering? Are you just rendering all prefixes on the server? Or are you passing the UA?
@nathanmarks We are sending the user agent to the browser, but I personally don't like relying on that. What about just putting them all in by default, and allow people to override them via props on muiTheme object, or the opposite, do none by default, and allow people to opt in.
muiTheme: {
// other stuff...
stylePrefixes: ['webkit', 'moz', 'ms']
}
@sorahn You'll be able to do whatever you want, prefix, not prefix, pass all
, pass a UA, etc.
@nathanmarks I have just released jss@4.0.0. Most important change - deterministic id's generation, here is an example of ssr with react http://jsstyles.github.io/examples/index.html
Just throwing in my $.02 here... while I really like the idea of inline styles and aphrodite appeals to me, it almost feels like it would be easier if the styles were based in a library format in scss like bootstrap 4.
What I usually do is literally copy the bootstrap.scss file and the _variables.scss file while creating a _mixins.scss file... with that copy under ./lib/style, I update the local bootstrap.scss file to reference the local variables and mixins, while using the path to the node_modules's versions of everything else...
I then setup my webpack config to make ./lib/style part of the search path for the sassLoader config.
In this way, I can have a main.scss file that loads bootstrap, etc... I can from there reference variables and/or mixins within the rest of my project, and when I build/bundle the output, the appropriate styles are included.
The down side to this, is it would prescribe using material-ui as source (not pre-build) and webpack for including scss in the bundles. That said, it would probably be the most straight forward path of using existing variables, classes and styling mixins.
Again, I really like the idea of JS styling, and tbh like what Aphrodite has to offer... I was working through something similar (was going to call it derma) as a styling library for inline styles, but Aphrodite already does everything I planned on and more.
Is the styling solution finalized?
Are there any updates on this?
Yep, the next
branch is using jss
@callemall/material-ui please leave some input here when you can π
We need to decide on a styling solution for
0.16.0
that will help address long standing issues. Outside of performance issues, there are hacks and props all over the place to make up for the fact that we are missing out on some of the powerful features in CSS that cannot be used with inline styles -- pseudo classes, media queries (without matchmedia), etc.From what I understand, the general consensus is that we want a JS style solution that has the capability to write styles to an actual stylesheet in the DOM.
Here are a few of the maintained solutions that do this: https://github.com/rofrischmann/react-look https://github.com/Khan/aphrodite https://github.com/jsstyles/react-jss
Here are some points we need to consider when implementing the new solution (IMO):
If using a library as large asI realized that 9kb of that is https://github.com/rofrischmann/inline-style-prefixer which we already use... πreact-look
, try and see how we can import modules for a minimal build size. The full build size is 16kb gzipped which is fairly large. It would be great if we can minimize the impact on build size.