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

RFC: Move styling to react-look #141

Open
mattkrick opened this issue Apr 19, 2016 · 15 comments
Open

RFC: Move styling to react-look #141

mattkrick opened this issue Apr 19, 2016 · 15 comments

Comments

@mattkrick
Copy link
Owner

Styling is always a pain. However, I think moving to react-look would make them less of a pain.
https://github.com/rofrischmann/react-look
general info on inline styling: http://engineering.khanacademy.org/posts/aphrodite-inline-css.htm (aphrodite is very similar to react-look)

Pros:

  • Same API for React Native
  • Uses pure CSS behind the scenes for media queries & pseudos, so it's fast
  • No need for another HTTP call just for the stylesheet
  • Send modular css, not a big prerendered dump (like what we currently do)
  • Faster webpack build times

Cons:

  • can't use react-dom-stream with it (or maybe you can? prove me wrong, please!)
  • ?
@wtgtybhertgeghgtwtg
Copy link

I can't seem to tell from the docs, but how will it play with Content Security Policy?

@mattkrick
Copy link
Owner Author

what's your concern with CSP?

@tychota
Copy link

tychota commented Apr 20, 2016 via email

@wtgtybhertgeghgtwtg
Copy link

wtgtybhertgeghgtwtg commented Apr 20, 2016

I'm not a fan of style-src unsafe-inline. The extract-text-webpack-plugin can extract style elements to stylesheets, but depending on how react-look does its stateful conditions, it might not be sufficient.

On another note, how will this issue work with #108?

@mattkrick
Copy link
Owner Author

ohhhh! inline styles has nothing to do with unsafe-inline. it just means you write your css in a .js file.

For 3rd party css files (leaflet, font-awesome, graphiql, etc) I'm not sure what the best thing to do is... probably stick em in a <style> tag? Hopefully push em back to load em as lazily as possible.

@patrickleet
Copy link
Contributor

@mattkrick Ever get into this? Any solution for the issues with react-dom-stream?

@wtgtybhertgeghgtwtg
Copy link

Issues with react-dom-stream might not be easy to work with. Officially, it doesn't play nice with React 15, so that's not making it any easier.
aickin/react-dom-stream#19 (comment)

@mattkrick
Copy link
Owner Author

Yep, react-look is implemented in Action here: github.com/parabolinc/action

Action is exactly like meatier, except I'm being paid to build it, so it'll probably take over meatier in a few months. (Although it uses auth0 instead of DIY auth).

Dom streaming just isn't possible though since the styles are rolled up into 1 large style tag for SSR. I mean, I could generate the HTML string and stream that on my own, but I just don't know how much benefit that will have. With the goal of just sending out HTML above the fold, probably very little.

@patrickleet
Copy link
Contributor

Awesome, thanks for the info, I'll look at Action!

@patrickleet
Copy link
Contributor

@mattkrick how does routesOrPrerender() work?

@mattkrick
Copy link
Owner Author

@patrickleet 😳 oh wow, you actually did look into it!

react-look stores all the styles in a singleton. So, if all the routes are in their own client-only webpack bundle, and the singleton is created with another version of react-look, we're not gonna catch em all. Getting rid of the singleton would be really, really difficult... a friend & i burned about 4 hours trying to do that. So instead, I merged the prerender code in with the client bundle.
However, that's not good enough because we still need to use the client-only routes to figure out what route to SSR. So, we could build a client-only bundle and a client-only+prerender bundle, but an entirely new webpack build means an extra 10-40 seconds compile time. bleh.
So, since the routes is a subset of client-only+prerender, i made it dynamic so you can chose the subset or the superset.

hope that makes sense. it's not glamorous, but it works & build times are screaming fast.

@patrickleet
Copy link
Contributor

http://fela.js.org/

@mattkrick
Copy link
Owner Author

I am very excited about fela. All the goodness of react-look without the bloat! We'll be switching over our production app as soon as the dust settles.

@patrickleet
Copy link
Contributor

Things are changing fast! (per usual) Yea it looks pretty awesome. I was about to start converting some radium stuff to react look and saw that posted on the react-look repo, thinking I might as well just jump straight into fela intstead.

@mattkrick
Copy link
Owner Author

most definitely, a couple cobwebs are starting to creep up on react-look, which is definitely understandable... nobody likes to maintain a library they no longer use. keep me posted on how you like it!

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

4 participants