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

Move from react-look to fela or aphrodite #124

Closed
mattkrick opened this issue Jul 20, 2016 · 5 comments
Closed

Move from react-look to fela or aphrodite #124

mattkrick opened this issue Jul 20, 2016 · 5 comments

Comments

@mattkrick
Copy link
Member

Issue - Bug

React 15.2 has pretty much killed react-look, and the package is unmaintained, so we need something different.
The successor to react-look is fela.
A lightweight version put out by the good folks at khan academy is aphrodite

Right now, my preference would be to go with aphrodite for the following reasons:

  • I'm not sure I like the fela API, where you have to create your children before you can use them: http://fela.js.org/docs/guides/UsageWithReact.html
  • aphrodite is older & has a larger community
  • aphrodite is smaller
  • concerning dynamic css (a fela feature), aphrodite handles it like: this, which may actually be more performant if we memoize that function call.
  • concerning fallback values (another fela feature), we aren't targeting older browsers, so we don't need the extra logic
  • aphrodite is front-end agnostic, which means new versions of react won't break it (15.2 😞 ). To be fair, Fela is front-end agnostic, too, but it has a a react wrapper that currently is suffering from the 15.2 passdown props problem, so we'd need to do a PR to get up and running.

However, at the end of the day, I'll go with @ackernaut's preference, since he'll use it a lot more than I will.

@mattkrick
Copy link
Member Author

@mattkrick
Copy link
Member Author

mattkrick commented Jul 27, 2016

Also, consider where to place style.
Currently, we define styles above the component (per eslint), then put the bulk of them below.
However, style constants then must also live above the component, making us eat a component sandwich: (https://github.com/ParabolInc/action/blob/master/src/universal/components/UserHub/UserHub.js#L7-L10).

So we have options:

  • component sandwich
  • define below
  • define all styles in another file

@mattkrick
Copy link
Member Author

when we move, we should also put the theme in the react context if there's no way to set it at compile time.

@mattkrick
Copy link
Member Author

Another option is JSS:

After reading both, i'm still bullish on aphrodite

@ackernaut
Copy link
Member

Thanks for dropping those two posts here @mattkrick —I read them, going to let that simmer until we start looking more closely at this again.

@mattkrick mattkrick mentioned this issue Oct 3, 2016
@jordanh jordanh closed this as completed in 681a5dd Oct 4, 2016
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

No branches or pull requests

2 participants