Spruce - Angela F. #69
Conversation
Digital starter
updated json file
There was a problem hiding this comment.
Nice job!
All tests (including the optional reset tests) are passing, once I removed the remaining skips.
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.
Also, it's currently possible to continue playing after a winner was found, even resulting in a change in winner. We need to make sure not to respond to clicks once the game is over.
But overall, well done!
| const updateSquareLetter = () => { | ||
| props.onClickCallback(props.id); | ||
| }; |
There was a problem hiding this comment.
Nice custom handler to call our click callback when the button was clicked. Notice that we can't simply use props.onClickCallback directly as the onClick handler, since we need to pass the id of the clicked square to the callback. If we had used it as the onClick directly, then the event details provided by the browser would have been passed in as the first parameter rather than the id.
| const PLAYER_1 = 'X'; | ||
| const PLAYER_2 = 'O'; | ||
| const PLAYER_1 = 'x'; | ||
| const PLAYER_2 = '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 it looks like you worked it out yourself!
| const resetGame = () => { | ||
| // Complete in Wave 4 | ||
| setSquares(generateSquares()); | ||
| setWinner(null); | ||
| setCurrentPlayer(PLAYER_1); | ||
| }; |
There was a problem hiding this comment.
Yes, resetting the game essentially comes down to setting the state back to its initial values.
| // }; | ||
|
|
||
| const onClickCallback = (id) => { | ||
| setSquares((squares) => { |
There was a problem hiding this comment.
Yes, we can use use the callback style of updating the squares here, but we don't actually know whether we want to or not. What if the player clicked on a square that was already played on? What if the game was already over? In that case, we could track whether a play was made, and if not, simply return the original board reference.
|
|
||
| const onClickCallback = (id) => { | ||
| setSquares((squares) => { | ||
| let emptyBoard = squares.map((square)=>{ |
There was a problem hiding this comment.
Good idea to copy the existing squares state using map, 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 on line 120 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.
| "plugin:jsx-a11y/recommended", | ||
| "plugin:react-hooks/recommended", | ||
| "plugin:jest/recommended", | ||
| "plugin:testing-library/react" |
There was a problem hiding this comment.
Removing this line does help reduce the warnings in the test files, but notice that doing so leaves a trailing comma (line 14), which isn't permitted in JSON.
| for (let openSquare of square){ | ||
| if (openSquare.id === id && openSquare.value === ''){ | ||
| openSquare.value = currentPlayer; | ||
| // if (!winner) { |
There was a problem hiding this comment.
Without this check (and without protecting the openSquare assignment as well) it's possible to continue playing the game even after there's a winner, and the winner can even change! You could add this check as part of the checks on line 107.
Notice that we should have still duplicated the inner array (each row of the board) and made a copy of the square being modified, adding the copied, updated square to the new row.
| } | ||
| return square; | ||
| }); | ||
| setWinner(checkForWinner()); |
There was a problem hiding this comment.
As written, checkForWinner should not be called during the click handling. It is written to look at the squares piece of state, but if we are properly calculating a new state and setting it, then the updated squares state wouldn't become observable until the next render. It is working here, since the changes to the board values are cross-contaminating the current React squares without React knowing about it (due to JS storing references to data).
Instead, with how checkForWinner is written, it should be called in the body of the App component function, which would check for a winner on each re-render. Notice this would allow us to avoid introducing a piece of state to track the winner.
3 participants
No description provided.