Codebase review project #1
Conversation
JohnWat92
force-pushed
the
codebase-review-project
branch
from
13116e6
to
79e4bd9
Compare
| import FlatButton from 'material-ui/FlatButton'; | ||
| import styles from './styles.css'; | ||
| import { sortNewest, sortPopular } from '../../redux/actions'; | ||
|
|
There was a problem hiding this comment.
Grouping related imports will help you stay organised. Decide how you're going to group and make sure you're consistent across all files
eg. In this file:
import React from 'react';
import { connect } from 'react-redux';
import { sortNewest, sortPopular } from '../../redux/actions';
import { Toolbar, ToolbarTitle } from 'material-ui/Toolbar';
import FlatButton from 'material-ui/FlatButton';
import styles from './styles.css';
| } | ||
| } | ||
|
|
||
| export default Week; |
There was a problem hiding this comment.
Nice job on this component. Separating your .map functions into Class Methods is the way to go.
| color: red; | ||
| font-family: Roboto, sans-serif; | ||
| padding-left:255px; | ||
| /*margin: 1rem;*/ |
There was a problem hiding this comment.
Remove all commented code from your project submissions.
| this.sortPopular = this.sortPopular.bind(this); | ||
| this.sortNewest = this.sortNewest.bind(this); | ||
| this.state = { | ||
| posts: data.posts, |
There was a problem hiding this comment.
I know we refactored this together, but please ensure you remove all local state from your components, when using Redux.
| } | ||
| } | ||
| const mapStateToProps = (state) => ( { posts: state.posts }); | ||
| export default connect(mapStateToProps)(PostListContainer); |
There was a problem hiding this comment.
You mapped the same state to a parent & child component (PostListContainer.js & PostList.js). This is not necessary. Map state to the component that needs it. Try not to pass state down as a prop on a component, unless absolutely necessary. Be careful not to map the same state to parent & child components as this could cause unnecessary re-rendering of your component tree and is generally redundant and confusing.
| export const updateVote = id => ({ type: UPDATE_VOTE, id }); | ||
| export const sortNewest = posts => ({ type: SORT_NEWEST, posts }); | ||
| export const sortPopular = posts => ({ type: SORT_POPULAR, posts }); | ||
|
|
There was a problem hiding this comment.
It's not necessary to pass in posts to your sorting action creators. (sortNewest & sortPopular) You can access state.posts inside the reducer. Action creators don't always need an argument and actions don't always need to have a payload. Sometimes it's simpler if they're just actions with a type, like in this case. Please refactor your action creators.
| console.log(' sorting newest', state); | ||
| console.log('state', state); // gives me the right thing | ||
| console.log('this', this); //undefined | ||
| // console.log('this.state', this.state); |
There was a problem hiding this comment.
Please remove all console.log's and comments from submitted projects.
No description provided.