Spruce- Ivette F. #67
Conversation
…set button, changed app tests to have capital X and O but still not passing and at this point I cannot be bothered
There was a problem hiding this comment.
Nice job!
All tests (including the optional reset tests) are passing. Sorry you had to update all of the tests to use capital letters! And I like the game over styling you applied, though unfortunately, it does confuse the tests a bit!
The main thing to watch out for here is to avoid modifying the current data in some piece of state without telling React about it. Your click handler is modifying the current squares value, which is shared both by the previous state and the new state. It doesn't lead to a problem here, but watch out for that on larger, more complex projects where it might be possible for various parts of an app to appear out of sync with each other.
But overall, well done!
| const PLAYER_1 = 'X'; | ||
| const PLAYER_2 = 'O'; | ||
| const player1 = 'X'; | ||
| const player2 = 'O'; |
There was a problem hiding this comment.
We gave bad guidance here. This should have been
const PLAYER_1 = 'x';
const PLAYER_2 = 'o';since the tests were all written for lower case x and o. But you got around this by modifying your tests to use capital letters.
| const onClickCallback = (markedSquare) => { | ||
|
|
||
| const makeNewBoard = (squares) => { | ||
| const newBoard = [...squares]; |
There was a problem hiding this comment.
Good idea to copy the existing squares state, but notice this only does a shallow copy. So we get a new outer array, but the inner arrays (the rows) and the square data objects are shared between the previous state, and the new state. This is what allows the checkForWinner call here in the click handler to appear to work. It references the squares state variable, which shouldn't appear to be updated until the next render. The fact that it is correctly seeing the winning move means that our code here is cross contaminating the previous state rather than only making a new state.
| @@ -35,31 +35,97 @@ const App = () => { | |||
| // When it is clicked on. | |||
| // Then pass it into the squares as a callback | |||
|
|
|||
| // How to get playerTurn to reach Square where the square state gets updated? | |||
| const [playerTurn, setPlayerTurn] = useState(player1); | |||
| const [playerTurns, setPlayerTurns] = useState(0); | |||
There was a problem hiding this comment.
This piece of state doesn't appear to be used for anything.
| const newBoard = [...squares]; | ||
| for (let row of squares) { | ||
| for (let square of row) { | ||
| if ((square.id === markedSquare.id) && (square.value === '')) { |
There was a problem hiding this comment.
These checks are spot on. The only issue is that they result in modifying the current state and the next state, since the squares are shared between both (only the outer array was copied).
| const winner = checkForWinner(); | ||
| if (winner) { | ||
| setWinnerState(winner); | ||
| } |
There was a problem hiding this comment.
This is only working because of the state cross contamination. But if we move the checkForWinner call to after it gets defined, and change the variable name to winnerState (and remove the useState and related stuff) the rest just works. (We'd also need to clean up the cross contamination for a full fix).
| const resetGame = () => { | ||
| // Complete in Wave 4 | ||
| setSquares(generateSquares()); | ||
| setPlayerTurn(player1); | ||
| setPlayerTurns(0); | ||
| setWinnerState(null); | ||
| }; |
There was a problem hiding this comment.
Yes. Reseting the game is simply a matter of setting all the pieces of state back to their initial values.
| playerTurn: PropTypes.string.isRequired, | ||
| winnerState: PropTypes.oneOf(['X', 'O', null]) |
There was a problem hiding this comment.
Nice adding these proptypes, but notice that the tests didn't expect these to be here, resulting in some errors.
| playerTurn: PropTypes.string.isRequired, | ||
| winnerState: PropTypes.oneOf(['X', 'O', null]) |
There was a problem hiding this comment.
Here too, nice to see these added, but it does cause problems for the Square tests.
| return <button disabled={props.winnerState !== null} | ||
| onClick={onSquareClick} className='square'>{props.value}</button>; |
There was a problem hiding this comment.
Rather than disabling the buttons (based on whether the game is over) we can add logic to the click handler back in App to prevent processing any clicks after the game is over. That would allow us to handle the required functionality without needing to introduce additional props (which causes problems for the tests).
In your own application, your approach would be perfectly reasonable and is up to you.
No description provided.