Accelerate - Denisse Anaya #2
Conversation
There was a problem hiding this comment.
Nice job!
Your tests are all passing, and your code is laid out well.
Be a little bit careful about how state that refers to mutable containers (arrays, objects). Here, that would be the squares data. As Rect applications become more complicated, we should strive to not modify existing state (the contents of squares) in the process of building the next state. For an app with a small number of state pieces, if we are a little untidy about what view of state we hold it's probably not going to become a problem, but for larger applications, it might lead to unexpected behavior. To help in this regard, there are libraries and entire frameworks to manage changes.
Overall, good work!
|
|
||
| return <button | ||
| className="square" | ||
| > | ||
| key={props.id} |
There was a problem hiding this comment.
The key here isn't necessary. We only need to add a key on the top-level component being rendered in an array. In this case, that would be up in the Board where the squares data is being mapped to a list of Square components (you have the key set correctly there).
| @@ -2,7 +2,7 @@ div.grid { | |||
| width: 600px; | |||
| height: 600px; | |||
| margin: 0 auto; | |||
| background-color: #34495e; | |||
| background: linear-gradient(blue, pink); | |||
There was a problem hiding this comment.
This was a really nice, subtle touch.
| // of square components | ||
|
|
||
| } | ||
| const lineSquares = [].concat(...squares); |
There was a problem hiding this comment.
At first glance, it can be difficult to understand what this expression actually does. To be more self-documenting, consider using the flat() method.
| return <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:
return <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!
| const PLAYER_1 = 'x'; | ||
| const PLAYER_2 = 'o'; |
There was a problem hiding this comment.
Yep. I don't know why the tests want lowercase. 🤣
|
|
||
| while (i < 3) { |
There was a problem hiding this comment.
If I know exactly how many times I want a loop to return, I tend to prefer using a for loop.
for (let i = 0; i < 3; ++i) {The variable declaration, check, and update are all grouped together.
| } | ||
| } | ||
| } | ||
| updatedGrid.push(squares[i]); |
There was a problem hiding this comment.
By adding the current row to the new array, this is enough for React to notice the change and trigger a re-render, but do be aware that these rows are still shared with the current square state for the duration of the render. This is why the checkWinner function is able to operate using the squares state value, even though the next render hasn't happened. Modifying the rows here modifies the rows for the new data, as well as the current data.
In this case, since there's not too much else going on, this isn't really a problem, but if we were working on a larger system where additional code might be running still before the render occurs, having the state change could result in unexpected behavior.
| return; | ||
| currentSquare.value = player; | ||
| if (player === PLAYER_1) { | ||
| setPlayer(PLAYER_2) |
There was a problem hiding this comment.
(obligatory "be careful of semicolons" comment)
| updatedGrid.push(squares[i]); | ||
| } | ||
| setSquares(updatedGrid); | ||
| setWinner(checkForWinner); |
There was a problem hiding this comment.
Be a little careful with this call to setWinner. We want to set the winner value to the result of evaluating checkForWinner. This code is passing in a reference to the checkForWinner. This happens to work due to the alternative setState usage we discussed in roundtables:
setState(currentValue => {
const newValue = doSomethingToCurrentValue(currentValue);
return newValue;
});JS doesn't complain if the number of arguments don't match, so it tries to pass the current value into checkForWinner, which get ignored. But calling checkForWinner returns a new value so it all ends up working out.
No description provided.