From cb6fd85c7f1fb8bed8cc52a9eefdfc3ce49f4e74 Mon Sep 17 00:00:00 2001 From: Hunter McMillen Date: Tue, 11 Apr 2017 17:10:30 -0400 Subject: [PATCH 1/8] Update this repo to use the new NameGame API endpoint Summary of changes: 1. Now using `https://willowtreeapps.com/api/v1.0/profiles` as the new endpoint 2. Updating helper functions to use new JSON response format 3. Submit PR to willowtreeapps.com repo to pull all people for the namegame API endpoint, not just the first 100 that Contentful returns by default: https://github.com/willowtreeapps/willowtreeapps.com/pull/439 TODO: 1. Some people don't have images in contentful, should this be left as is or filtered out 2. The api endpoint only returns the first 100 'people' objects from Contentful due to the way requests are paged, additional work will be needed to get all people in a single response 3. Matt fixed a CORS bug in the willowtreeapps.com routing logic to add the correct CORS headers for cached and non-cached requests to the namegame endpoint. This won't be available until the next website release, so for the time being only the first request will succeed (https://github.com/willowtreeapps/willowtreeapps.com/pull/438) --- index.html | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/index.html b/index.html index b9bef462..508ebb3d 100644 --- a/index.html +++ b/index.html @@ -96,19 +96,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,12 +124,18 @@ ***************************************************/ - function getLastName(fullName) { - return fullName.match(/\w+/g)[1]; + function 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}`; }; /** @@ -157,7 +164,7 @@ */ function filterByName(searchForName, personList) { return personList.filter((person) => { - return person.name === searchForName; + return person.firstName === searchForName || person.lastName === searchForName; }); } @@ -199,7 +206,7 @@ }; } - const sortByFirstName = sortObjListByProp('name'); + const sortByFirstName = sortObjListByProp('firstName'); const sortByLastName = (personList) => sortByFirstName(personList).reverse(); @@ -220,10 +227,10 @@ 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' }, [ From c63cfa4aaef369ea5b625be856d8bd8a1aa90436 Mon Sep 17 00:00:00 2001 From: Rebecca Pledger Date: Fri, 22 Sep 2017 19:00:51 -0500 Subject: [PATCH 2/8] :tada: Add Bootstrap and modify CSS Update styling with Bootstrap v3.3.7 and make modifications to CSS for improved UI - Add Bootstrap CDN link - Add Bootstrap classes to buttons and table. - Add spacing in between objects - Change width to 50% to be more responsive --- index.html | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/index.html b/index.html index 508ebb3d..34603cf8 100644 --- a/index.html +++ b/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 } @@ -79,7 +82,7 @@ - + - + + From 293e273861f40556c321e0f9bd118c8ee8676eaf Mon Sep 17 00:00:00 2001 From: Rebecca Pledger Date: Fri, 22 Sep 2017 22:02:18 -0500 Subject: [PATCH 6/8] :goat: Modify shuffle function to not mutate data & fix slice(1) bug It is a React best practice to not mutate data used as props or state. The result variable is used to store the copy of list, however slice(1) was copying without index 0. Change slice(1) to slice(0) to fix this. Inorder to not mutate the list data, the result variable must also be used in the for loop when performing the shuffling. - Modify slice(1) to slice(0) to include index 0 - Use result variable in shuffle for loop for swap --- index.html | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/index.html b/index.html index b64febdf..97df34e9 100644 --- a/index.html +++ b/index.html @@ -146,15 +146,15 @@ */ 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; From 6f83c0bf73c83eb045402f86ff89d7bf3bd3eb7a Mon Sep 17 00:00:00 2001 From: Rebecca Pledger Date: Fri, 22 Sep 2017 22:42:21 -0500 Subject: [PATCH 7/8] :art: Make getLastName consistent with getFirstName Use ES6 Javascript for getFirstName function to match getLastName function --- index.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/index.html b/index.html index 97df34e9..49755502 100644 --- a/index.html +++ b/index.html @@ -127,7 +127,7 @@ ***************************************************/ - function getLastName(person) { + const getLastName= (person) => { return person.lastName; } From 84aed0bd65afbe64824dc324de39a46a6769a4d6 Mon Sep 17 00:00:00 2001 From: Rebecca Pledger Date: Fri, 22 Sep 2017 23:15:46 -0500 Subject: [PATCH 8/8] :memo: Add Code Review notes in CodeReview.md Document all changes proposed in Code Review with explainations of change reasoning. --- CodeReview.md | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 CodeReview.md diff --git a/CodeReview.md b/CodeReview.md new file mode 100644 index 00000000..636f1984 --- /dev/null +++ b/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. \ No newline at end of file