Code Review

CodeReview.md

@@ -0,0 +1,38 @@
+# Code Review Notes:
+
+## Update NameGame API
+Include commit from feature branch to update to latest NameGame API: https://willowtreeapps.com/api/v1.0/profiles.
+
+## Add Boostrap and modify CSS
+Updating the UI helps make the application much easier to use. Using Bootstrap, the UI can be easily update to achieve a friendlier look and feel. I updated to Bootstrap v3.3.7 using CDN link in html and made some modifications to CSS. I made the following modifications to the UI:
+
+* Add Boostrap classes to buttons and table to add Bootstrap styling
+* Add spacing in between objects, such as buttons and table, to make the UI more friendly
+* Change width to 50% to be more responsive (app size will update with page resize)
+
+## Sorting
+The functionality for first name sorting works correctly, but last name sorting does not work properly. Instead of reversing the first name sort, called the sortObjListByProp() function with the last name prop. The slice(1) commands is supposed to copy the full array, but it does not include the element at index 0. Changing this to slice(0) allows the code to include element 0. I also added a new feature for sorting which allows an optional tiebreaker prop for breaking a tie in the comparator when the primary prop is equal. I used this by calling the function with firstName as the primary prop and lastName as the tie breaker prop. Below is a summary of the changes:
+
+* Fix last name sort with sortObjListByProp()
+* Copy full array with slice(0) instead of slice(1)
+* Add tie breaker feature to break ties when primary prop is equal
+
+## Searching
+Add regular expression for searching at the start of a string to implement real-time searching. Before, the search function would not return results until the full first name or last name was entered. Now it is updated with search results each time a character is typed. I decided to search at the start of first name OR the start of last name, which seemed like an intuitive way for someone to go about searching for names. Below is a summary of the changes:
+
+* Create regex for searchForName at start of string
+* Filter on start of first name or start of last name
+
+## Upgrde React
+Update to lastest versions of React and React.Dom.
+* React v15.6.1
+* React.Dom v 15.6.1
+
+## Shuffle
+One React best practice to not mutate date used as props or state. Not mutating props or state data also improves the performance of the application. In the shuffle function, a copy is made of the list prop using the slice(1) command. However, similarly to in sorting, slice(1) must be changed to slice(0) in order to copy full array (including element 0). The result variable (copy of list prop) was created, but was not actually used in the shuffle algorithm. In order to not mutate the list data, the result variable must also be used in the shuffle algorithm for loop. Below is a summary of the changes:
+
+* Change slice(1) to slice(0) to include index 0
+* Use result variable in shuffle for loop for swap
+
+## getLastName consistency
+Use latest ES6 Javascript notation for getFirstName function, which also makes it consistent with the getFirstName function. It is a good practice to maintain consistency in syntax even if both are accepted. 

index.html

@@ -56,8 +56,8 @@
         }
 
         .app-container {
-            width: 400px;
-            margin: 0 auto 0 auto;
+            width: 50%;
+            margin: 20px auto 0 auto;
         }
 
         .image {
@@ -67,7 +67,10 @@
 
         .list-container {
             width: 100%;
-            border: 1px solid black;
+            margin-top: 20px;
+        }
+        .btn {
+            margin-left: 10px
         }
         </style>
     </head>
@@ -77,9 +80,9 @@
 
 
         <!-- 3rd Party Scripts -->
-        <script src="https://cdnjs.cloudflare.com/ajax/libs/react/0.14.7/react.js"></script>
-        <script src="https://cdnjs.cloudflare.com/ajax/libs/react/0.14.7/react-dom.js"></script>
-
+        <script src="https://cdnjs.cloudflare.com/ajax/libs/react/15.6.1/react.js"></script>
+        <script src="https://cdnjs.cloudflare.com/ajax/libs/react/15.6.1/react-dom.js"></script>
+        <link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/css/bootstrap.min.css" integrity="sha384-BVYiiSIFeK1dGmJRAkycuHAHRg32OmUcww7on3RYdg4Va+PmSTsz/K68vbdEjh4u" crossorigin="anonymous">
 
         <!-- Our Application Logic -->
         <script>
@@ -96,19 +99,20 @@
          * The data comes back in the format:
          *
          *    [
-         *        { name: 'Viju Legard', url: '...' },
-         *        { name: 'Matt Seibert', url: '...' },
+         *        { firstName: 'Viju,  lastName: 'Legard',  headshot: { url: '...' } },
+         *        { firstName: 'Matt', lastName: 'Seibert', headshot: { url: '...' } },
          *        ...
          *    ]
          */
         function getPersonList() {
             return new Promise((resolve, reject) => {
-                fetch('http://api.namegame.willowtreemobile.com/').then(function(response) {
+                fetch('https://willowtreeapps.com/api/v1.0/profiles')
+                .then(response => {
                     if (response.status !== 200) {
                         reject(new Error("Error!"));
                     }
 
-                    response.json().then((imageList) => {
+                    response.json().then(imageList => {
                         resolve(imageList);
                     });
                 });
@@ -123,28 +127,34 @@
          ***************************************************/
 
 
-        function getLastName(fullName) {
-            return fullName.match(/\w+/g)[1];
+        const getLastName= (person) => {
+            return person.lastName;
         }
 
-        const getFirstName = (fullName) => {
-            return fullName.match(/\w+/g)[0];
+        const getFirstName = (person) => {
+            return person.firstName;
+        };
+
+        // headshot URLs are scheme relative //
+        // prepend http: to prevent invalid schemes like file:// or uri://
+        const getImageUrl = (person) => {
+            return `http:${person.headshot.url}`;
         };
 
         /**
          * Fisher-Yates shuffle
          */
         function shuffleList(list) {
             // Make a copy & don't mutate the passed in list
-            let result = list.slice(1);
+            let result = list.slice(0);
 
             let tmp, j, i = list.length - 1
 
             for (; i > 0; i -= 1) {
                 j = Math.floor(Math.random() * (i + 1));
-                tmp = list[i];
-                list[i] = list[j];
-                list[j] = tmp;
+                tmp = result[i];
+                result[i] = result[j];
+                result[j] = tmp;
             }
 
             return result;
@@ -154,10 +164,13 @@
         /**
          * Remove any people that do not have the name we are
          * searching for.
+         *
+         * Perform RegEx on Start of first name or last name
          */
         function filterByName(searchForName, personList) {
+            var regexp = new RegExp('^' + searchForName, 'i')
             return personList.filter((person) => {
-                return person.name === searchForName;
+                return regexp.test(person.firstName) || regexp.test(person.lastName);
             });
         }
 
@@ -172,24 +185,29 @@
          *    const sortPeopleByName = sortObjListByProp('name');
          *    const sortedPeople = sortPeopleByName(people);
          *
+         * Also takes in an optional tiebreaker property to break ties when prop is equal
+         *
          *  We now have:
          *
          *    console.log(sortedPeople)
          *    > [{ name: 'Jon' }, { name: 'Kevin' }, { name: 'Sam' }]
          *
          */
-        function sortObjListByProp(prop) {
+        function sortObjListByProp(prop, tiebreaker) {
             return function(objList) {
                 // Make a copy & don't mutate the passed in list
-                let result = objList.slice(1);
+                let result = objList.slice(0);
 
                 result.sort((a, b) => {
                     if (a[prop] < b[prop]) {
                         return -1;
-                    }
-
-                    if (a[prop] > b[prop]) {
+                    }else if (a[prop] > b[prop]) {
                         return 1;
+                    }else if(tiebreaker){
+                        if (a[tiebreaker] > b[tiebreaker]){
+                            return 1;
+                        }
+                        return -1;
                     }
 
                     return 1;
@@ -199,9 +217,9 @@
             };
         }
 
-        const sortByFirstName = sortObjListByProp('name');
+        const sortByFirstName = sortObjListByProp('firstName', 'lastName');
 
-        const sortByLastName = (personList) => sortByFirstName(personList).reverse();
+        const sortByLastName = sortObjListByProp('lastName', 'firstName');
 
 
         /*==================================================
@@ -220,13 +238,13 @@
             src: props.src
         });
 
-        const ListRow = (props) => React.DOM.tr({ key: props.person.name }, [
-            React.DOM.td({ key: 'thumb' }, React.createElement(Thumbnail, { src: props.person.url })),
-            React.DOM.td({ key: 'first' }, null, getFirstName(props.person.name)),
-            React.DOM.td({ key: 'last' }, null, getLastName(props.person.name)),
+        const ListRow = (props) => React.DOM.tr({ key: `${props.person.firstName} ${props.person.lastName}` }, [
+            React.DOM.td({ key: 'thumb' }, React.createElement(Thumbnail, { src: getImageUrl(props.person) })),
+            React.DOM.td({ key: 'first' }, null, getFirstName(props.person)),
+            React.DOM.td({ key: 'last' }, null, getLastName(props.person)),
         ]);
 
-        const ListContainer = (props) => React.DOM.table({ className: 'list-container' }, [
+        const ListContainer = (props) => React.DOM.table({ className: 'list-container table' }, [
             React.DOM.thead({ key: 'thead' }, React.DOM.tr({}, [
                 React.DOM.th({ key: 'thumb-h' }, null, 'Thumbnail'),
                 React.DOM.th({ key: 'first-h' }, null, 'First Name'),
@@ -281,9 +299,9 @@
 
                 return React.DOM.div({ className: 'app-container' }, [
                     React.createElement(Search, { key: 'search', onChange: this._onSearch }),
-                    React.DOM.button({ key: 'shuffle', onClick: this._shuffleList }, null, 'Shuffle'),
-                    React.DOM.button({ key: 'sort-first', onClick: this._sortByFirst }, null, 'Sort (First Name)'),
-                    React.DOM.button({ key: 'sort-last', onClick: this._sortByLast }, null, 'Sort (Last Name)'),
+                    React.DOM.button({ key: 'shuffle', onClick: this._shuffleList , className: 'btn btn-default'}, null, 'Shuffle'),
+                    React.DOM.button({ key: 'sort-first', onClick: this._sortByFirst, className: 'btn btn-default' }, null, 'Sort (First Name)'),
+                    React.DOM.button({ key: 'sort-last', onClick: this._sortByLast, className: 'btn btn-default' }, null, 'Sort (Last Name)'),
                     React.createElement(ListContainer, { key: 'list', personList: visiblePersonList })
                 ]);
             }