-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Implement React [email protected] #393
Changes from 12 commits
19046a0
24d5343
441faac
5c3c6c2
d3e51e2
c75ff80
ce00c61
b761625
8fa5ca5
558727d
164b862
694ddac
3e2cfc5
5667b09
180e669
2e5ce1d
33e87de
a194252
a1b920e
c91dea4
b8fa9ad
82f1ee4
70c1d9c
578176d
aa1fb15
707e12a
a2b6bca
2e2d60d
407144e
3f56197
a5ef8d2
c6609c0
1e72aaa
3fc6175
ab0e2a3
408632f
46b7d26
27169b3
c35174f
f651fc5
057033a
bd13289
a5cf0e0
26387b8
8ad2d6e
57a54e7
e0a8524
8d68161
e7c9c3f
95e6c48
e665193
c155977
d94c0ee
fb0902d
d116756
251fba8
3efc625
975fe9e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
import React from 'react'; | ||
import {Link} from 'react-router'; | ||
|
||
// Link is no longer a part of the 'react-router v4 API' | ||
import { Link } from 'react-router-dom'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. which formatting does our linter support? Spacing between {} in imports or none? I can't remember. cc: @kwelch. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure, I know I prefer with spaces. I also believe prettier default is with spaces. |
||
import '../styles/about-page.css'; | ||
|
||
// Since this component is simple and static, there's no parent container for it. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,20 +1,37 @@ | ||
import React, { PropTypes } from 'react'; | ||
import { Link, IndexLink } from 'react-router'; | ||
import { Route } from 'react-router'; | ||
import { Switch, NavLink } from 'react-router-dom'; | ||
// 'Switch' & 'NavLink' imports have moved from the 'react-router' API to 'react-router-dom' since rrV4; | ||
// Also, IndexLink is replaced by NavLink with the activeStyle attr. | ||
// Switch is required for NotFoundPage to be the last case scenario. | ||
|
||
import HomePage from './HomePage'; | ||
import FuelSavingsPage from '../containers/FuelSavingsPage'; // eslint-disable-line import/no-named-as-default | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider removing this eslint-disable, your code meets the import/no-named-as-default rule afaict. |
||
import AboutPage from './AboutPage'; | ||
import NotFoundPage from './NotFoundPage'; | ||
|
||
// This is a class-based component because the current | ||
// version of hot reloading won't hot reload a stateless | ||
// component at the top-level. | ||
|
||
class App extends React.Component { | ||
render() { | ||
const activeStyle = { color: 'blue' }; | ||
return ( | ||
<div> | ||
<IndexLink to="/">Home</IndexLink> | ||
{' | '} | ||
<Link to="/fuel-savings">Demo App</Link> | ||
{' | '} | ||
<Link to="/about">About</Link> | ||
<br/> | ||
{this.props.children} | ||
<div> | ||
<NavLink exact to="/" activeStyle={activeStyle}>Home</NavLink> | ||
{' | '} | ||
<NavLink to="/fuel-savings" activeStyle={activeStyle}>Demo App</NavLink> | ||
{' | '} | ||
<NavLink to="/about" activeStyle={activeStyle}>About</NavLink> | ||
</div> | ||
<Switch> | ||
<Route exact path="/" component={HomePage} /> | ||
<Route path="/fuel-savings" component={FuelSavingsPage} /> | ||
<Route path="/about" component={AboutPage} /> | ||
<Route component={NotFoundPage} /> | ||
</Switch> | ||
</div> | ||
); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,20 +1,22 @@ | ||
import React, { Component, PropTypes } from 'react'; | ||
import { Provider } from 'react-redux'; | ||
import routes from '../routes'; | ||
import { Router } from 'react-router'; | ||
import { ConnectedRouter } from 'react-router-redux'; | ||
import App from './App'; | ||
|
||
export default class Root extends Component { | ||
render() { | ||
const { store, history } = this.props; | ||
return ( | ||
<Provider store={store}> | ||
<Router history={history} routes={routes} /> | ||
<ConnectedRouter history={history}> | ||
<App /> | ||
</ConnectedRouter> | ||
</Provider> | ||
); | ||
} | ||
} | ||
|
||
Root.propTypes = { | ||
store: PropTypes.object.isRequired, | ||
history: PropTypes.object.isRequired | ||
history: PropTypes.object.isRequired, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider removing trailing comma There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @oshalygin lol, i seem to be the only one who hasn't disabled that rule in .eslintrc files. Newest commit has both changes you suggested. Thanks for the feedback! |
||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,20 @@ | ||
import {createStore, compose, applyMiddleware} from 'redux'; | ||
import reduxImmutableStateInvariant from 'redux-immutable-state-invariant'; | ||
import thunk from 'redux-thunk'; | ||
import createHistory from 'history/createBrowserHistory'; | ||
// 'routerMiddleware': the new way of storing route changes with redux middlware since rrV4. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing "e" in middleware |
||
import { routerMiddleware } from 'react-router-redux'; | ||
import rootReducer from '../reducers'; | ||
|
||
export const history = createHistory(); | ||
function configureStoreProd(initialState) { | ||
const reactRouterMiddleware = routerMiddleware(history); | ||
const middlewares = [ | ||
// Add other middleware on this line... | ||
|
||
// thunk middleware can also accept an extra argument to be passed to each thunk action | ||
// https://github.com/gaearon/redux-thunk#injecting-a-custom-argument | ||
thunk, | ||
reactRouterMiddleware, | ||
]; | ||
|
||
return createStore(rootReducer, initialState, compose( | ||
|
@@ -19,6 +24,7 @@ function configureStoreProd(initialState) { | |
} | ||
|
||
function configureStoreDev(initialState) { | ||
const reactRouterMiddleware = routerMiddleware(history); | ||
const middlewares = [ | ||
// Add other middleware on this line... | ||
|
||
|
@@ -28,6 +34,7 @@ function configureStoreDev(initialState) { | |
// thunk middleware can also accept an extra argument to be passed to each thunk action | ||
// https://github.com/gaearon/redux-thunk#injecting-a-custom-argument | ||
thunk, | ||
reactRouterMiddleware, | ||
]; | ||
|
||
const composeEnhancers = window.__REDUX_DEVTOOLS_EXTENSION_COMPOSE__ || compose; // add support for Redux dev tools | ||
|
@@ -49,4 +56,4 @@ function configureStoreDev(initialState) { | |
|
||
const configureStore = process.env.NODE_ENV === 'production' ? configureStoreProd : configureStoreDev; | ||
|
||
export default configureStore; | ||
export default configureStore; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a bit rough to be using an alpha package. Many of us are using this library for projects and these things will inevitably break into beta+
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I'd like to avoid taking on alpha/beta dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh didn't see this one here still. @TobiahRex, can the RR4 PR use a stable version of
react-router-redux
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about replace react-router-redux (which is still in alpha) with connected-react-router ?