Code Review #5
Closed
Code Review #5
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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: willowtreeapps/willowtreeapps.com#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 (willowtreeapps/willowtreeapps.com#438)
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
Sorting by first name works well, but sorting by last name just reverses the first name sort, which is incorrect. Fix this by calling the sortObjListByProp() function with lastName prop. The slice(1) command intended to copy full array, but removes index 0. Call slice(0) instead to copy full array. Add a new feature to sort which adds an optional tiebreaker prop for when primary prop is equal. - Fix last name sort with sortObjListByProp() - Copy full array with slice(0) instead of slice(1) - Add tiebreaker feature to break ties when primary prop is equal
Add RegEx for start of string to search real-time instead of searching on full first name or full last name. Filter on either start of first name or start of last name. - Create regex for searchForName at start of string - Filter on start of first name or start of last name
Upgrade to the following versions: - React v15.6.1 - React.Dom v15.6.1
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
Use ES6 Javascript for getFirstName function to match getLastName function
Document all changes proposed in Code Review with explainations of change reasoning.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The commits in this pull request contain the proposed changes for my code review. The CodeReview.md contains detailed descriptions of the changes.