Grading PR - Comments and Feedback [DO NOT MERGE]

README.md

@@ -1 +1,3 @@
-# app
+# app
+
+WRITEME.md

src/components/AllColorsStatic.js

@@ -1,33 +1,19 @@
 import React from 'react';
 
-export default function AllColor(props) {
+const Color = (src, alt) => (
+    <div className="three columns">
+        <img src={`/assets/${src}.svg`} alt={alt}/>
+    </div>
+);
+
+export default function AllColor() {
     return (
         <div className='container'>
             <div className='row'>
-                <div className="three columns">
-                    <img
-                        src='/assets/blue1.svg'
-                        alt='sad, dark blue'
-                        />
-                </div>
-                <div className="three columns">
-                    <img
-                        src='/assets/green1.svg'
-                        alt='relaxed, dark green'
-                        />
-                </div>
-                <div className="three columns">
-                    <img
-                        src='/assets/yellow1.svg'
-                        alt='happy, dark yellow'
-                        />
-                </div>
-                <div className="three columns">
-                    <img
-                        src='/assets/red1.svg'
-                        alt='angry, dark red'
-                        />
-                </div>
+                <Color src='blue1' alt='sad, dark blue'/>
+                <Color src='green1' alt='relaxed, dark green'/>
+                <Color src='yellow1' alt='happy, dark yellow'/>
+            { /* etc */}
             </div>
             <div className='row'>
                 <div className="three columns">

src/components/AllUsers.js

@@ -46,11 +46,7 @@ export default class AllUsers extends Component {
         })
         .then(fullCities => {
             cities = fullCities.map(city => {
-                const splitCity = city.split(',');
-                if(splitCity.length > 1){
-                    return splitCity.pop();
-                }
-                return city;
+                return city.split(',')[0];
             });
         })
         .then(() => {
@@ -98,7 +94,6 @@ export default class AllUsers extends Component {
         });
     }
 
-
     populateFilter(chooseDefault, type, stateFilter) {
         return (
             <select className='four columns offset-one' onChange={(e) => {
@@ -135,14 +130,12 @@ export default class AllUsers extends Component {
     }
 
     onDateChange(date) {
-        console.log(date)
         return fetcher({
             path: `/users/moods/date?date=${date}`,
             method: 'GET',
         })
             .then(res => res.json())
             .then(moods => {
-                console.log(moods)
                 this.setState({
                     moods,
                 });
@@ -165,12 +158,13 @@ export default class AllUsers extends Component {
                     <div className='eight columns offset-by-two'>
                         <span className='margin-label'>Filter by date:</span>
                         <input type='date' ref='date' onChange={(e) => {
+                            // because you have e, I think you can just use e.target.value
                             const date = this.refs.date.value;
                             e.preventDefault();
                             this.onDateChange(date);
                         }}/>
                         <button className='button-primary float-right' onClick={(e) => {
-                            e.preventDefault();
+                            e.preventDefault(); // is this needed?
                             this.onAllMoods();
                         }}>All Moods</button>
                     </div>

src/components/App.js

@@ -64,16 +64,19 @@ class App extends Component {
     }
     render() {
         return (
-            < Router >
-                < div className='app'>
-                    < Route path='/' render={props => (<NavBar {...props} isSignedIn={this.state.isSignedIn} handleSignOut={this.handleSignOut}/>)} />
-                    < MainBody 
+            <Router >
+                <div className='app'>
+                    <Route path='/' render={props => (<NavBar {...props} isSignedIn={this.state.isSignedIn} handleSignOut={this.handleSignOut}/>)} />
+                    <MainBody 
                         isSignedIn={this.state.isSignedIn} 
                         token={this.state.token} 
                         handleSignIn={this.handleSignIn}/>
-                    < Route path='/' component={ Footer }/>
+                    { /* this Route isn't need as is always going to match
+                        and your not using props */
+                    }
+                    <Footer/>
                 </div>
-            </ Router >
+            </Router >
         );
     }
 }

src/components/Footer.js

@@ -1,6 +1,6 @@
 import React from 'react';
 
-export default function Footer(props) {
+export default function Footer() {
     return (
         <footer>
             2017colordiary

src/components/HomePage.js

@@ -1,18 +1,26 @@
-import React from 'react';
+import React, { Component } from 'react';
 import AllColorsStatic from './AllColorsStatic';
 //import ReactCSSTransitionGroup from 'react-addons-css-transition-group';
 
-export default function HomePage(props) {
+// You could encapsulate (though fine how you had it too) like:
+class ColorLink extends Component {
+    render() {
+        return <a style={{color: this.props.color}}>{this.children}</a>;
+    }
+}
+
+export default function HomePage() {
+
     return (
         <section className='container'>
             <div>
                 <div className='title'>
                     <h3>your mood,</h3>  
                     <h1>                  
-                        <a style={{color: '#8181FC'}}>H</a>                     
-                        <a style={{color: '#6CB1BC'}}>U</a> 
-                        <a style={{color: '#F2B451'}}>E</a>
-                        <a style={{ color: '#E55C5C' }}>D</a>
+                        <ColorLink color='#8181FC'>H</ColorLink>                     
+                        <ColorLink color='#6CB1BC'>U</ColorLink> 
+                        <ColorLink color='#F2B451'>E</ColorLink>
+                        <ColorLink color='#E55C5C'>D</ColorLink>
                     </h1>
                 </div>
                 < AllColorsStatic />

src/components/MainBody.js

@@ -7,14 +7,17 @@ import AllUsers from './AllUsers';
 import { Route, Redirect } from 'react-router-dom';
 
 export default function MainBody({ handleSignIn, isSignedIn, token }) {
+    // IMO, easier to read this not burried in the list of Route below
+    const UserComponent = props => isSignedIn 
+        ? <UserMain {...props} isSignedIn={isSignedIn} token={token}/>
+        : <Redirect to={{  pathname:'/signin', state: { from: props.location }}}/>;
+
     return (
         <main>
             < Route exact path='/' component={ HomePage } />
             < Route path='/signup' render={props => (<SignUp {...props} handleSignIn={handleSignIn}/>)} />
             < Route path='/signin' render={props => (<SignIn {...props} isSignedin={isSignedIn} handleSignIn={handleSignIn}/>)}/>
-            < Route path='/user' render={props => (
-                isSignedIn ? (<UserMain {...props} isSignedIn={isSignedIn} token={token}/>) 
-                : (<Redirect to={{  pathname:'/signin', state: { from: props.location }}}/>))}/>
+            < Route path='/user' render={UserComponent}/>
             < Route path='/allusers' component={ AllUsers } />
         </main>
     );

src/components/NavBar.js

@@ -2,6 +2,7 @@ import React from 'react';
 import { Link } from 'react-router-dom';
 
 export default function NavBar({ handleSignOut, isSignedIn }) {
+
     return (
         <nav className='container'>
             <ul className='twelve columns'>

src/components/UserCommentView.js

@@ -4,7 +4,7 @@ import { formatDate } from '../helpers/formatDate';
 
 export default function UserCommentView(props) {
     const dayBlocks = props.allMoods;
-    console.log('dayBlocks',dayBlocks);
+
     let weatherMood;
     if(dayBlocks) {
         let dayMoods = dayBlocks.filter(block => {
@@ -15,7 +15,7 @@ export default function UserCommentView(props) {
         weatherMood = dayMoods[0];
     }
     const formattedDate = formatDate(props.date);
-    console.log('comment date', formattedDate);
+    // this is a run-on render, find a way to break it up so it's more readable
     return (
         <div className='container'>
             <h5 className='text-center'>{formattedDate}</h5>

src/components/UserMain.js

@@ -34,36 +34,33 @@ export default class UserMain extends Component {
         const token = localStorage.getItem('token'); 
         let date= currentDateToString();
 
-        fetcher({ 
-            path: '/user', 
-            method: 'GET', 
-            token: token 
-        })
-        .then(res => {
-            return res.json();
-        })
-        .then(user => {
+        // these should be parallel
+        Promise.all([
+            fetcher({ 
+                path: '/user', 
+                method: 'GET', 
+                token: token 
+            }),
             fetcher({
                 path: '/block', 
                 method: 'GET', 
             })
-            .then(res => {
-                return res.json();
-            })
-            .then(blocks => {
-                this.setState({
-                    blocks,
-                    allMoods: blocks,
-                    date,
-                    user
-                });
-
-            })
-            .then (() => {
-                this.doFetchDate();
+        ])
+        .then(([user, blocks]) => {
+            this.setState({
+                blocks,
+                allMoods: blocks,
+                date,
+                user
             });
+
         })
-        .catch()
+        // could this be in parallel too?
+        .then (() => {
+            this.doFetchDate();
+        });
+        // this is pointless:
+        // .catch()
     }
 
     handleBlockSelect(chosenBlock) {
@@ -77,8 +74,7 @@ export default class UserMain extends Component {
         this.setState({
             date
         }, () => {
-            this.doFetchDate()
-
+            this.doFetchDate();
         })
     }
 
@@ -89,9 +85,6 @@ export default class UserMain extends Component {
             method: 'GET', 
             token
         })
-        .then(res => {
-            return res.json();
-        })
         .then(moods => {
             const allMoods = [...this.state.blocks];
             moods.forEach((mood) => {
@@ -148,5 +141,4 @@ export default class UserMain extends Component {
         );
     }
 }
-                // <span>Today's weather: </span>
-                // <span>Location: </span>
+

src/components/UserMonthChart.js

@@ -2,6 +2,9 @@ import React from 'react';
 import RC2 from 'react-chartjs2';
 
 export default function UserMonthChart(props) {
+    // There's a lot of repetative work happening here.
+    // You need to figure out what the right data structures
+    // are that are needed.
     const moodColors = getColors(props.monthColors);
     const pieColor = Object.keys(moodColors);
     const moodCount = getCount(moodColors);
@@ -17,6 +20,7 @@ export default function UserMonthChart(props) {
 
 
     function count(monthColors) {
+        // this is a .reduce, reduce the array down to a single result (accumulator)
         const colorCount = {};
         monthColors.forEach((monthColor) => {
             let indexColor = monthColor.color.hexColor;
@@ -27,13 +31,15 @@ export default function UserMonthChart(props) {
     }
 
     function values(monthColorsCount) {
+        // this is a map, go through the fist array and return a corresponding value
         const colorValues = [];
         Object.keys(monthColorsCount).forEach((key) => {
             colorValues.push(monthColorsCount[key]);
         });
         return colorValues;
     }
 
+    // isn't this the same as "function count" above???
     function getColors(monthColors) {
         const colorCount = {};
         monthColors.forEach((monthColor) => {

src/components/UserMonthView.js

@@ -37,18 +37,11 @@ export default class UserMonthView extends Component {
             method: 'GET',
             token
         })
-        .then(res => res.json())
-        .then(moods => {
-            monthColors = moods.map(mood => {
-                return mood.color;
-            })
-            return moods;
-        })
         .then(moods => {
             this.setState({
                 monthMoods: moods,
-                monthColors 
-            })
+                monthColors: moods.map(mood => mood.color)
+            });
         })
         .catch(err => {
             console.log('userMonth', err)
@@ -60,7 +53,6 @@ export default class UserMonthView extends Component {
             date
         }, () => {
             this.doFetchMonth()
-
         })
     }
 

src/components/UserMoodSelector.js

@@ -66,14 +66,13 @@ export default class UserMoodSelector extends Component {
             },
             token,
         })
-        .then(res => {
-            return res.json();
-        })
         .then(json => {
             if(json.error) {
                 console.log('error', json.error);
                 return;
             }
+            // consider integrating the new data into the data you 
+            // already have rather than having to refetch
             this.props.doFetchDate();
         });
     }

src/components/UserMoodsDay.js

@@ -9,6 +9,8 @@ export default class UserMoodsDay extends Component {
         match: PropTypes.object.isRequired,
     }
 
+    // this method is a bit of a mess. It needs to be broken into some 
+    // chunks to make it easier to understand.
     render() {
         const formattedDate = formatDate(this.props.date);
         if(!this.props.allMoods && !this.props.date) {