feedback: code review comments

app/components/ChooseBall.jsx

@@ -3,6 +3,7 @@ import ReactDOM, { render } from 'react-dom';
 import { Link, NavLink, Router } from 'react-router-dom'
 import Firebase from 'firebase';
 
+// OB/JL: watch out for relative paths in client side!
 const balls = [
   { name: 'Heavy Duty', description: 'Ball fashioned by the vikings themselves.', img: './assets/textures/grayball-choose.png' },
   { name: 'Sleuth', description: "You like things that move with the grace of a cheetah.", img: './assets/textures/netball-choose.png' },  
@@ -11,6 +12,7 @@ const balls = [
 class ChooseBall extends React.Component {
   constructor(props) {
     super(props)
+    // OB/JL: consider arrow function class syntax
     this.ballChoice = this.ballChoice.bind(this)
   }
 
@@ -20,6 +22,7 @@ class ChooseBall extends React.Component {
   }
 
   ballChoice(evt) {
+    // OB/JL: consider putting the send-to-firebase logic into chooseBall
     this.props.chooseBall(+evt.target.id)
     this.sendDataToFB()
   }

app/components/Game.jsx

@@ -13,6 +13,7 @@ const database = firebase.database();
 const objects = [];
 const thisPlayer = '';
 const playersInGame = {};
+// OB/JL: maybe wrap all this up into an object called babylonState
 let sceneNum = 1;
 let torus;
 let winPos;
@@ -29,18 +30,20 @@ class Game extends Component {
     super(props);
   }
 
+  // OB/JL: split it up! it's too big to not fail!
   componentDidMount() {
     audio0.play();
-    const user = this.props.user.userId;
+    const user = this.props.user.userId; // OB/JL: maybe name this userId
     const canvas = this.refs.renderCanvas;
     const engine = new BABYLON.Engine(canvas, true);
     let num = sceneNum;
     let scene = createScene1(canvas, engine);
     database.ref('winPosition').set({ x: 10, z: 10 });
     this.createWinPoint();
     database.ref('winPosition').on('value', (position) => {
-      winPos = position.val();
+      winPos = position.val(); // OB/JL: couldn't this be a setState
     });
+    // OB/JL: name this as a function called "generateWinner" or something
     database.ref('players/winner').on('value', (winner) => {
       if (winner.val()) {
         if (user === winner.val()) {
@@ -64,6 +67,7 @@ class Game extends Component {
         database.ref('players/winner').remove();
       }
     });
+    // OB/JL: maybe only listen for child_added
     database.ref('players').on('value', (players) => {
       const playersObj = players.val();
       for (const playerId in playersObj) {
@@ -86,6 +90,7 @@ class Game extends Component {
               }
             });
           }
+          // OB/JL: camera stuff could be its own function
           const followCamera = new BABYLON.FollowCamera('followCam', new BABYLON.Vector3(0, 15, -45), scene);
           if (playerId === user) {
             const playerDummy = this.createCameraObj(scene, newPlayer);
@@ -108,14 +113,15 @@ class Game extends Component {
           playersInGame[playerId] = true;
         }
       }
+      // OB/JL: put this into a child_removed (?) listener
       for (let i = 0; i < objects.length; i++) {
         if (playersObj[objects[i].id].remove) {
           objects[i].dispose();
         }
       }
     });
     database.ref('players/' + user).set({ 'created': true, 'score': 0, 'remove': false });
-
+    // OB/JL: consider putting bablyon stuff into its own module and here, you'd just do e.g. `babylonEngine.init(canvasElement)`
     engine.runRenderLoop(() => {
       if ((torus.position.x !== winPos.x) || (torus.position.z !== winPos.z)) {
         torus.position.x = winPos.x;
@@ -182,6 +188,8 @@ class Game extends Component {
     database.ref('playerPosition/' + user).remove();
     database.ref(user).remove();
     audio0.pause();
+    // OB/JL: don't forget to clean up all your listeners
+    // OB/JL: shouldn't this stop the engine?
   }
 
   createPlayerOnConnect(sce, id) {
@@ -197,6 +205,7 @@ class Game extends Component {
     return player;
   }
 
+  // OB/JL: nicecly wrapped up
   setPosition(sphere, x, y, z) {
     sphere.position.x = x;
     sphere.position.y = y;
@@ -232,6 +241,7 @@ class Game extends Component {
     torus.position.x = 10;
     torus.position.z = 10;
   }
+  // OB/JL: dead code
   makeId() {
     let text = '';
     const possible = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789';

app/components/Home.jsx

@@ -4,6 +4,7 @@ import ReactDOM, { render } from 'react-dom'
 const Home = () => (
     <section id="contain" className="hero">
     <div className="slider">
+    {/* OB/JL: make sure to include leading / in requests for files */}
     <img id="photoobj" className="media-object" src='assets/textures/blue_walkway_thin.png' />
     <div className="button is-success" id="overlay"><a href="#"> BUTTON </a></div>
     </div>

app/components/PrivateGameRoom.jsx

@@ -30,10 +30,12 @@ class GameWaitRoom extends React.Component {
         this.setState({ numberOfPlayers: players.val()[gameId].playersInGame.length });
       });
     } else {
+      // OB/JL: maybe move this into its own function or just into the componentWillUnmount directly
       firebase.database().ref('games').off();
     }
   }
 
+  // OB/JL: consider renaming to `togglePublic`
   security = () => {
     this.setState({ publicGame: !this.state.publicGame });
   }
@@ -46,7 +48,7 @@ class GameWaitRoom extends React.Component {
   }
 
   render() {
-    const numPlayer = 1;
+    const numPlayer = 1; // OB/JL: dead code
     return (
       <div>
         <div className="content has-text-centered">
@@ -71,7 +73,7 @@ class GameWaitRoom extends React.Component {
             type="submit"
             title="publicPrivate"
             onClick={() => { this.security(); }}>
-            {this.state.publicGame &&
+            {this.state.publicGame && // OB/JL: consider ternary
             <i className="fa fa-unlock"> Public</i>
             }
             {!this.state.publicGame &&

app/main.jsx

@@ -1,4 +1,4 @@
-'use strict';
+'use strict'; // OB/JL: might not be necessary
 import React from 'react';
 import {BrowserRouter as Router, Switch, Route, Redirect} from 'react-router-dom';
 import {render} from 'react-dom';
@@ -27,6 +27,8 @@ import Demos from 'APP/demos';
 const auth = firebase.auth();
 var loginObj = {};
 
+// OB/JL: this could be in a componentDidMount (or something)
+// OB/JL: consider making a `withAuth` higher-order react function
 auth.onAuthStateChanged((user) => {
   if (!user) {
     firebase.auth().signInAnonymously().catch(function(error) {
@@ -35,7 +37,7 @@ auth.onAuthStateChanged((user) => {
   } else {
     const ref = firebase.database().ref('users/' + user.uid);
     ref.once('value', (snapshot) => {
-      Object.assign(loginObj, user);
+      Object.assign(loginObj, user); // OB/JL: this could become a `setState` call
     });
   }
 });

app/reducers/auth.js

@@ -2,6 +2,7 @@ import axios from 'axios';
 import firebase from 'firebase';
 import store from '.././store.js';
 
+// OB/JL: consider flattening this to just an object, no `user` key
 const initialState = {
   user: {}
 };

app/reducers/conditionals.js

@@ -1,5 +1,6 @@
 const initialState = { render: false };
 
+// OB/JL: routing should be able to cover this (instead of redux)
 const SHOW_GAME_LIST = 'SHOW_GAME_LIST';
 export const showGameList = (render) => ({ type: SHOW_GAME_LIST, render });
 

comments.md

@@ -0,0 +1,7 @@
+- Use issues
+- Commit message: sync up on how you want to do it
+- Put deps in package.json and node_modules (e.g. babylon and cannon) and import them somewhere in main.jsx
+- Here's what you should do with dead code: 🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥
+- Delete demos folder (dead code)
+- Maybe ditch redux if it's getting in the way
+- Tests?!

public/index.html

@@ -31,6 +31,7 @@
   <!-- <p id="load">Firebase SDK Loading&hellip;</p> -->
 
   <script>
+    // OB/JL: dead code
     // document.addEventListener('DOMContentLoaded', function () {
     //   // // 🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥🔥
     //   // // The Firebase SDK is initialized and available here!

public/styles.css

@@ -1,3 +1,4 @@
+/* OB/JL: inconsistent indentation here */
 .headersec {
   background-color: #191b21;
   padding-bottom: 1%;
@@ -71,6 +72,7 @@ body {
   border-color: #1BC98E;
 }
 
+/* OB/JL: ids are the least re-usable */
 #neon {
   background-color: #1BC98E;
 }

secret.js

@@ -1,3 +1,4 @@
+// OB/JL: not secret, also dead code
 module.exports = {
   apiKey: "AIzaSyD5CuKXBrzpUH_m8ovpCdXCNbQ5uRKkPmI",
   authDomain: "myach-fsa.firebaseapp.com",