-
Notifications
You must be signed in to change notification settings - Fork 0
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
code-review #6
base: new-code-review
Are you sure you want to change the base?
code-review #6
Conversation
Updated readme 07132017
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.
You need to run eslint on your files
Remove all console.logs, and line spaces
.travis.yml
Outdated
- sudo -u postgres createuser bunmialao | ||
- createdb postit-test | ||
|
||
script: |
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.
you can omit these two lines
or use 'npm test' instead, there is no need to add "run" for scripts command that are native to npm scripts
Procfile
Outdated
@@ -0,0 +1 @@ | |||
web: NODE_ENV=production node server.prod.js |
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.
by default Heroku sets NODE_ENV to production, so, just node server.prod.js is fine
app.js
Outdated
import routes from './server/routes/index'; | ||
|
||
const port = parseInt(process.env.PORT, 10) || 3000; | ||
const homepage = `${__dirname}/client/index.html`; |
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.
unused variable
|
||
routes(app); | ||
|
||
module.exports = app; |
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.
export default app for consistency with es6 modules
client/Index.jsx
Outdated
import { createStore, applyMiddleware, compose } from 'redux'; | ||
import attachAuthorizationToken from "../client/utils/attachToken"; | ||
import store from '../client/store/configureStore'; | ||
import { setCurrentUser } from './actions/users'; |
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.
remember setCurrentUser is a default export, so remove curly braces
// import { userSignInRequest } from '../../actions/SignInAction'; | ||
import NavigationBar from '../NavigationBar'; | ||
|
||
class SignInPage 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.
use functional component
client/components/Sidebar.jsx
Outdated
import ListGroup from './Group/ListGroup'; | ||
|
||
|
||
const SideBar=() => ( |
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.
space before and after =
client/components/NavigationBar.jsx
Outdated
} | ||
|
||
} | ||
NavigationBar.proptypes = { |
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.
propTypes
} | ||
); | ||
|
||
const mapDispatchToProps = () => ({ |
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.
just use an object declaration here rather than using a function
|
||
listUsers (groupId) { | ||
if (this.props.users.length > 0) return; | ||
|
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.
remove line space here and below
No description provided.