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

Add Radium #13

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add Radium #13

wants to merge 2 commits into from

Conversation

paulyoung
Copy link
Contributor

This pull request intends to address #2.

Despite Radium not yet being compatible with React 0.13 (FormidableLabs/radium#92), I think there are other implications that can be discussed in the meantime.

The [Radium docs](https://github.com/formidablelabs/radium/blob/master/docs/guides/media-q
ueries.md) say this:

To start, choose a high level component in your app where you will initialize your media queries. Any components that should use your media queries should be descendents of this component.

Note: this means descendent as in view hierarchy, not class inheritance.

Since this project didn't have a single high level component, the first commit introduces an app router which is loosely based on the [shared root example](https://github.com/rackt/react-router/blob/master/examples/shared-root/a
pp.js) from React Router.

While there are further potential changes in this area to bring Essential React closer to the given example, those are not (yet) addressed here.

The second commit outlines how Radium could/would be added. These changes have been left as comments in order to keep the project compatible with 0.13 and allow any further refactoring of components.


Concerns

  • It seems odd to be setting up media queries inside a router but I'm not sure if/how a high level app component can be introduced in another way.
  • Although the MatchMediaItem mixin is being applied to pages I did wonder if it should instead be applied to LoggedInRouter and LoggedOutRouter.

Should the project be refactored and brought closer to the React Router example, some of these concerns might be redundant.


There are other aspects to Radium which have little (if any) implications and have yet to be implemented for that reason.

Looking for some input from @pheuter on how best to proceed.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.7% when pulling ff0cad8 on paulyoung:radium into 95df1fd on pheuter:master.

@paulyoung
Copy link
Contributor Author

Unfortunately ES6 launched without any mixin support. Therefore, there is no support for mixins when you use React with ES6 classes. Instead, we're working on making it easier to support such use cases without resorting to mixins.

Source: https://facebook.github.io/react/docs/reusable-components.html

@pheuter
Copy link
Owner

pheuter commented Mar 24, 2015

Nice! Will take a look when I get the chance. Some people have expressed that Radium may not be considered an "essential" part of a React app, especially considering its controversial approach to styling and potential lack of cross-browser support, which is why I haven't been too actively pursuing it. Nonetheless, the potential is still there and I think it may still have a place in this skeleton.

@paulyoung
Copy link
Contributor Author

Yeah, feel free to close if you want.

I still think the React Router example is worth a look even if Radium is not included.

@pheuter pheuter changed the title [WIP] Add Radium Add Radium Mar 24, 2015
@dmitry
Copy link

dmitry commented Apr 6, 2015

Any news on this topic?

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

Successfully merging this pull request may close these issues.

4 participants