-
Notifications
You must be signed in to change notification settings - Fork 1
After running generate with redux, local install #1
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
Conversation
rails generate react_on_rails:install --redux
42ab6f6
to
100a92a
Compare
export default class HelloWorldWidget extends React.Component { | ||
static propTypes = { | ||
// If you have lots of data or action properties, you should consider grouping them by | ||
// passing two properties: "data" and "actions". |
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.
I don't know if I really agree with that, sounds like sort of a "god" component that is doing too much, prefer using child smart components in this case
@@ -0,0 +1,8 @@ | |||
import actionTypes from '../constants/helloWorldConstants'; | |||
|
|||
export function updateName(name) { |
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.
usually these are written a bit more concisely using fat arrow:
export const updateName = (name) => ({ type: actionTypes.HELLO_WORLD_NAME_UPDATE, name });
import React, { PropTypes } from 'react'; | ||
|
||
// Simple example of a React "dumb" component | ||
export default class HelloWorldWidget extends React.Component { |
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 should be converted to an SFC, the only method we have is unnecessary:
onChange={(event) => props.updateName(event.target.value)}
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.
@nostophilia not yet done
</h3> | ||
<hr /> | ||
<form className="form-horizontal"> | ||
<label> |
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.
Are we linting? Usually this will warn about not specifying an htmlFor
. Either way we should do it as it's considered bad practice not to do so.
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.
I added the linter in PR #2.
|
||
const actionTypes = mirrorCreator([ | ||
'HELLO_WORLD_NAME_UPDATE', | ||
]); |
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 is fine, but I'm not sure it's still the best way to go and have moved away from it on my internal projects
// once your app has asynchronous actions. | ||
import thunkMiddleware from 'redux-thunk'; | ||
|
||
export default props => { |
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.
If I need to default export and want to use an arrow function, I always put it into a const
so that Babel gives the function a name (which helps in stack traces). This will show up as an anonymous function if there is an error.
Also, I like to name the argument railsProps
so that it's clear that this is not an SFC and that those props are coming from Rails.
"react-on-rails": "6.1.2", | ||
"react-redux": "^4.4.1", | ||
"redux": "^3.3.1", | ||
"redux-promise": "^0.5.3", |
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.
we don't need this
"name": "react-webpack-rails-tutorial", | ||
"version": "0.0.1", | ||
"engines": { | ||
"node": "5.10.0", |
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.
should we bump node?
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.
Yes on node.
"react-redux": "^4.4.1", | ||
"redux": "^3.3.1", | ||
"redux-promise": "^0.5.3", | ||
"redux-thunk": "^2.0.1", |
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.
We don't really need this either
"redux": "^3.3.1", | ||
"redux-promise": "^0.5.3", | ||
"redux-thunk": "^2.0.1", | ||
"redux-immutable": "^3.0.8", |
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.
We don't really need this
rails generate react_on_rails:install --redux