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

Uses css classes for styling instead of inline styles #283

Closed

Conversation

tuckerconnelly
Copy link

@tuckerconnelly tuckerconnelly commented Dec 12, 2016

This patch solves the following problem

Compatibility with Uranium and ultimately Carbon UI.

More generally, adds compatibility with classnames. Currently, any library that uses classnames to apply styles, or any user-provided css classes, will get overridden by RNW's inline styles.

This also pretty significantly reduces the css footprint, and I wouldn't be surprised if it also improved performance. material-ui noticed a 25% decrease in performance when moving to inline styles from classes.

Test plan

Updated the jest test snapshots, and confirmed it worked with the carbon ui documentation site.

This pull request

  • includes documentation
  • includes tests
  • includes an interactive example
  • includes screenshots/videos

@necolas
Copy link
Owner

necolas commented Dec 12, 2016

material-ui noticed a 25% decrease in performance when moving to inline styles from classes

Noticed similar on an internal mobile.twitter.com branch. Now that this library essentially has full support for RN styles and i18n, we can go back to introducing a more efficient className implementation that operates at the declaration level (which styletron recently emulated).

@remon-nashid
Copy link

remon-nashid commented Dec 12, 2016

I'm still new to React-native, so my question is mainly to learn.

I had the impression that js inline styling solves some CSS issues; such as eliminating dead code, avoid global scoping and minification. Also IMO it's a gateway to lots of fun programmable styling, such as react-native-extended-stylesheet.

I'm wondering what advantages does CSS bring. Also if it's still possible with this merge request to use inline styling besides CSS.

@remon-nashid
Copy link

Alright, checked the merge request as well as the underlying library (classnames). Seems that inline styling will still work and that library is actually interesting. Thanks!

@necolas
Copy link
Owner

necolas commented Dec 16, 2016

I'm closing this because it doesn't really solve the problem and creates others. Any styles set via RNW will still override the styles set via className. It only works if you use AppRegistry. And the classnames aren't obfuscated. I'll be working on a solution to the problem you raise. Thanks

@necolas necolas closed this Dec 16, 2016
@necolas
Copy link
Owner

necolas commented Jan 2, 2017

Hey, try using 0.0.62. Styles are resolved to className where possible—which should allow you to use Uranium—and rendering performance is about 13x better against the benchmarks.

@tuckerconnelly
Copy link
Author

Hey yup, it works :) Thanks. It is a little difficult to debug directly though, would be nice if in the future there could be a single classname per element.

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

Successfully merging this pull request may close these issues.

3 participants