Spruce: Kelemen - Tic Tac Toe Digital starter #68
Conversation
…t-tic-tac-toe into digital-starter
…rd deadline, tabling lifting state etc
There was a problem hiding this comment.
Nice job!
All tests (including the optional reset tests) are passing. And nice additions of the color styling, and trying out a controlled form (see my feedback on a few changes that can be made there).
Nice job using the checkForWinner in the flow of the component function rather than in immediate response to a click. Since the state value isn't updated until the next render, we can't correctly use the function during the click handler (unless we accidentally cross contaminate our new state with the current state). But the way you called checkForWinner respects the state properly.
Well done!
| <button type="submit" value="Add Player Name" onSubmit={onFormSubmit}> | ||
| Submit | ||
| </button> |
There was a problem hiding this comment.
We should use an input of type submit rather than button, and then but the submit handler on the form itself. Forms can be submitted a number of ways. Most usually, a submit button, but also, if a user is editing a text field and hits the return key! By moving the handler to the form, then regardless of how the form is submitted, our event handler will be called!
| <div> | ||
| <label htmlFor="player1">Player X name: </label> | ||
| <input | ||
| userName1="player1Name" |
There was a problem hiding this comment.
We can't add arbitrary attributes to tags. The input tag doesn't know what userName1 means (and an error is reported in the console). A more common pattern for inputs is to set their name attribute to match the name of the key holding their data in the form state. Doing so allows us to use a single input handler, since the name of the key we want to update can be read from the input name!
| return ( | ||
| <button | ||
| key={id} | ||
| style={value === 'x' ? { color: 'green' } : { color: 'red' }} |
There was a problem hiding this comment.
Nice use of a calculated style to provide the additional styling here. Though I would consider using the value to add an additional class, rather than an inline style, which would allow for more arbitrary styling using whatever styles were mapped to the class in the CSS file.
Additionally, consider passing the character used for player 1 as one of the props rather than hard coding the 'x' here. Prior to adding this check, Square was completely agnostic to the characters used to represent player 1 and player 2, but now it needs at least one of the players to be represented by 'x'.
Alternatively, consider moving the calculation of the additional class up to the game logic, and storing it with the square objects so that it could just be passed down. It's not clear to me which approach would be better in the long term (with Square doing the calculation, App doesn't even need to know this is occurring), but it's useful to consider additional options.
| color: rgb(199, 238, 199); | ||
| } | ||
|
|
||
| .red { | ||
| color: rgb(187, 133, 133); | ||
| } |
There was a problem hiding this comment.
These styles aren't being used, since the value-based Square styling is using inline styles.
|
|
||
| return null; | ||
| }; | ||
| const nameColor = player === PLAYER_1 ? 'green' : 'red'; |
There was a problem hiding this comment.
Nice calculated value to get the class to apply to the status line. No state required! We can simply do the calculation in the body of our function before we reach the return. Components are functions, and we we can run whatever code we need to generate our tag hierarchy before we get to the return statement!
| if (madeMove) { | ||
| setSquares(newState); | ||
| setPlayer(player === PLAYER_1 ? PLAYER_2 : PLAYER_1); | ||
| } |
There was a problem hiding this comment.
👍
We only want to switch players if there was a valid move made.
| }; */ | ||
| } | ||
|
|
||
| const winner = checkForWinner(); |
There was a problem hiding this comment.
Great. We can't make use of the checkForWinner (at least as written) during the click handler, since the state variable that it checks won't be updated until the subsequent render. But we can call it in the function body (which happens on each render) to see whether there is a winner, and then make adjustments to the tag structure we return based on the calculated winner (if any).
| const resetGame = () => { | ||
| // Complete in Wave 4 | ||
| setSquares(generateSquares()); | ||
| setPlayer(PLAYER_1); | ||
| }; |
There was a problem hiding this comment.
Yes. Resetting the game simply involves setting each of our state values back to their initial values.
No description provided.