Accelerate - Trisb G. #8
Conversation
There was a problem hiding this comment.
Nice job!
Your tests are all passing (with your updates) and your code is clear and well organized.
One thing to focus on going forward with React is the use of container values (arrays, dictionaries/objects) in state, and being very intentional about how much of a copy we make. Spreading a container type performs a shallow copy. That is, it makes a new outer object, but then all of the inner values have their references copied directly. This means that any changes we make to the new object members will also impact the values in the old object.
This might be okay for small applications, but if there is additional logic in a larger application, this could result in parts of the application being rendered with one idea of what the state looked like, but then finding that the values were changed out from underneath it. Here it wasn't a problem, but we should keep an eye out for this.
Overall, good work.
| @@ -24,7 +24,7 @@ | |||
| work correctly both with client-side routing and a non-root public URL. | |||
| Learn how to configure a non-root public URL by running `npm run build`. | |||
| --> | |||
| <title>React App</title> | |||
| <title>Tic Tac Toe</title> | |||
| @@ -39,21 +39,21 @@ describe('App', () => { | |||
| expect(header).toBeInTheDocument(); | |||
| }); | |||
|
|
|||
| test('Clicking on a grid button changes the text on it to an "x"', () => { | |||
| test('Clicking on a grid button changes the text on it to an "🦶"', () => { | |||
There was a problem hiding this comment.
Yes, the tests are explicitly coded to look for x and o (lower case), so any player customization necessarily requires the tests to be updated. We could imagine in a fuller implementation that maybe we would allow the player 1 and 2 values to be set via props which (similar to the board and square tests) would allow the app tests to pass in the desired pieces without affecting the pieces used during the actual game.
| const updatedSquare = { | ||
| id: props.id, | ||
| } | ||
| props.onClickCallback(updatedSquare) |
There was a problem hiding this comment.
We could implement the onClickCallback to take just the id rather than wrapping it in an object. Not a big deal one way or the other. We might prefer the wrapped version (this implementation) if there was a chance that we might need to send back additional data, but weren't currently certain.
| for(let row of squares) { | ||
| for (let square of row) { |
There was a problem hiding this comment.
In for/of loops, we can make the variable const (always prefer const unless our logic requires let, and even then, think about rewriting the logic to be able to use const).
There was a problem hiding this comment.
The nested loop here is a perfectly reasonable approach. Any help method that we call, or any spreading shenanigans would need to perform the same kinds of operations, but from a clarity-of-intent standpoint, check out the flat() method of arrays.
| <Square | ||
| key={square.id} | ||
| id={square.id} | ||
| value={square.value} | ||
| onClickCallback={onClickCallback} | ||
| />) |
There was a problem hiding this comment.
If you find it annoying that you need to repeat the names and values of these attributes consider something like the following:
<Square key={square.id} {...{...square, onClickCallback}} />which creates a new object with the spread contents of square, and value in onClickCallback being stored under the key onClickCallback. This new object is itself spread within the attribute list area of the Square component, which turn the key-value pairs into attributes with values on the component!
| for (let row of newSquares) { | ||
| for (let square of row) { |
There was a problem hiding this comment.
for/of loops can use const for the loop variable.
| // Wave 2 | ||
| // You will need to create a method to change the square | ||
| // When it is clicked on. | ||
| // Then pass it into the squares as a callback | ||
| const updateSquare = (updatedSquare) => { | ||
| const newSquares = [...squares] |
There was a problem hiding this comment.
Notice that spreading the squares array makes a shallow copy (new outer array, but the row arrays are the same references as the original squares array). This means that even though you are iterating over newSqaures, the changes made are also affecting the existing squares values, which is why the checkForWinner function works later on when referring to squares.
The way your logic is set up, it would be relatively straight-forward to switch over to nested map calls to sever the data relationship.
For a relatively small app like this, we're unlikely to get ourselves into trouble by doing this, but for larger applications, we should be very careful about modifying the existing the state values to avoid different areas of our logic having different ideas about what the current state is.
| } | ||
|
|
||
| const printWinner = () => { | ||
| if (winner == null) { |
There was a problem hiding this comment.
Because of the explicit null check, the first move each round doesn't display the player's turn info. Either change the value used for winner at the start of a game to null, or change this check to winner being falsy
if (! winner) {since both null and empty string are falsy.
Opting to submit with all test passing and I shall continue to work on my real toe version on another branch.Saw that Dena was able to successfully test changing her markers to emojis. Had to go test by test and fix each to handle the case of using emojis as markers.
I think I'll continue to work on a version that actually uses an image of a toe instead of an emoji foot (🦶) for my future portfolio site.