Fixing Various Open Issues #9
Conversation
There was a problem hiding this comment.
Awesome! Thanks for working on this.
I have a few small things. If you want to update your PR based on this feedback, great! If not, I'm happy to merge it now and one or the other of us (or both!) can do more later. I've commented on the code in-line.
On the commits themselves, I like how you've split them up! I really appreciate keeping separate changes separate.
My preference for commit messages is to have the names describe the change - ie, f5eeb11 might be better named "reset guess on new person". That way they make sense in the commit log without having to go look up "what is issue #5, again?" Tying it to the issue is still really important, however! In the body of the message I like to have the issue number, as well as the title, again so you can look at it offline. So, the overall commit message for that commit might be:
reset guess on new person
Introduce a new action for resetting the guess, and call it when
the user clicks "Next!".Issue #5 Clear guess status on new active person
Then, in the pull request description, if you say "Resolves #5", GitHub will automatically close the issue when the PR is merged.
See also tpope's blog post on git commit message best practices.
These are all small problems, though, and the app will be better off once this PR is merged, so thank you again!
src/action_creators.js
Outdated
|
|
||
| export function clearGuest() { | ||
| return { | ||
| type: "CLEARGUEST", |
There was a problem hiding this comment.
Why "Guest"? I would have expected "Guess".
src/api.js
Outdated
| // last_name: "Estridge", | ||
| // middle_name: "", | ||
| // person_id: 587 | ||
| // } |
There was a problem hiding this comment.
Adding placeholder data is a good idea! We probably shouldn't use real people, though. When I was initially developing, I used flower pictures from Wikipedia, but any kinda of non-personal data should be fine.
src/api.js
Outdated
| // person_id: 587 | ||
| // } | ||
| // ] | ||
| // //Comment this out before commits |
There was a problem hiding this comment.
We can probably leave this uncommented, since it's not overwriting or shadowing anything.
src/api.js
Outdated
| @@ -1,4 +1,30 @@ | |||
| // //Static Data for Front-End only testing & development | |||
| // let people = [ | |||
src/api.js
Outdated
| // // Stub Code to Work on Front-End only by not hitting the API | ||
| // let data = people[Math.ceil(Math.random() * 100) % people.length] | ||
| // return new Promise((res, rej) => res(data)); | ||
| // // Comment this out and un-comment actual implementation before commits |
There was a problem hiding this comment.
I wonder if we can have an environment variable to control if this function calls the API or uses the placeholder data? I have no idea how to do that in javascript!
| if (guess.get('status') === "correct") { | ||
| return ( | ||
| <div className="correct guess"> | ||
| Correct! | ||
| Correct! Their Full Name is {this.props.person.first_name} {this.props.person.last_name} |
There was a problem hiding this comment.
If you want to stay in the Immutable.js world, you could say this.props.person.get('first_name') - or, better, extract a person variable next to the guess, and then just person.get('first_name'). Then you wouldn't need to .toJS() below.
Issue jasonaowen#7 Fix or remove title element ID
Introduce a new action for resetting the guess, and call it when the user clicks "Next!". Issue jasonaowen#5 Clear guess status on new active person
Issue jasonaowen#4 Show full name when correct Issue jasonaowen#8 Clear person name on new active person
Starting npm with environmental variable REACT_APP_USE_TEST_DATA = true will not hit the api but will use local dummy test data.
|
Great! |
Resolves the issues below. Added dummy test data for if you are only developing the front end.
Resolves #4
Resolves #5
Resolves #7
Resolves #8