Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sarah J. Olivas - Spruce #66

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Sarah J. Olivas - Spruce #66

wants to merge 14 commits into from

Conversation

sjolivas
Copy link

No description provided.

Copy link

@anselrognlie anselrognlie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job!

All tests (including the optional reset tests) are passing.

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!

Comment on lines +13 to +15
onClick={() => {
props.onClickCallback(props.id);
}}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice inline 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.

Comment on lines -6 to +7
const PLAYER_1 = 'X';
const PLAYER_2 = 'O';
const PLAYER_1 = 'x';
const PLAYER_2 = 'o';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 took care of this on your own!

Comment on lines +126 to +130
// resets squares state and player state.
const resetGame = () => {
// Complete in Wave 4
setSquares(generateSquares());
setPlayer(PLAYER_1);
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. Resetting the game essentially comes down setting the pieces of state back to their initial values.

Comment on lines +134 to 139
const statusLine = () => {
if (winner) {
return `Winner is ${winner}`;
}
return `${player}'s turn`;
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice helper to update the status line for the player turn or game winner.

Comment on lines +50 to +53
// doesn't allow anymore moves after a win.
if (winner) {
return;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can check the winner due to closure semantics (winner is defined in the outer execution context).

Comment on lines +57 to +70
const updateSquares = squares.map((row) =>
row.map((pos) => {
if (pos.id !== id || pos.value !== '') {
return pos;
}

// valid move if passes all checks
move = true;

// create new array that includes played square(s)
const playedSquare = { ...pos, value: player };
return playedSquare;
})
);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good use of double mapping and spreading the player square so that when a move is made, the new state is no longer connected to the old state (avoid state cross contamination).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants