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

Rae and Mac Tic Tac Toe #65

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

Rae and Mac Tic Tac Toe #65

wants to merge 16 commits into from

Conversation

raeelwell
Copy link

No description provided.

Copy link

@alope107 alope107 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 and very cute! There's a few bugs here, the biggest being that if you click on a square multiple times you can flip it back and forth between the two kitties. This also causes problems for your winner checking. However, the overall logic is still solid. Nicely done, this is a good display of your React skills!

border-radius: 24px;
}
h2 {
color: rgb(0, 0, 0);

Choose a reason for hiding this comment

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

Consider using named colors if the RGB vals exactly match one. For example, rgb(0, 0, 0) is the same as black.

Comment on lines +4 to +5
import matthew from './img/matthewface.png';
import junior from './img/juniorface.jpg';

Choose a reason for hiding this comment

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

💜

Comment on lines +32 to +42
return {
c048: [],
c012: [],
c345: [],
c678: [],
c036: [],
c147: [],
c258: [],
c642: [],
};
};

Choose a reason for hiding this comment

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

Very interesting method of tracking combos! In the future, consider adding a comment to explain how unconventional data structures like this are meant to be used so the code is easier to read.

Comment on lines +51 to +64
const updatedSquareData = squares.map((row) => {
return row.map((square) => {
if (square.id === id) {
if (player) {
square.value = PLAYER_1;
checkForWinner(square.id, square.value);
} else {
square.value = PLAYER_2;
checkForWinner(square.id, square.value);
}
}
return square;
});
});

Choose a reason for hiding this comment

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

Nice use of nested maps

</header>
<main>
<Board squares={squares} />
<Board squares={squares} updateSquares={updateSquares} />

Choose a reason for hiding this comment

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

Nice job passing down callback

Comment on lines +10 to +11
display: grid;
grid-template: repeat(3, 1fr) / repeat(3, 1fr);

Choose a reason for hiding this comment

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

😻

const squaresArray = singleArray.map((square) => {
return (
<Square
key={square.id}

Choose a reason for hiding this comment

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

Good job remembering key!

Comment on lines +7 to +8
const PLAYER_1 = <img src={matthew} alt="X" />;
const PLAYER_2 = <img src={junior} alt="O" />;

Choose a reason for hiding this comment

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

In the future, consider having further separation between data and presentation. Keeping the variables as X and O and changing only the rendering logic to show the kitties may be a bit cleaner. It also would have allowed you to still pass the automated tests.


Board.propTypes = {
squares: PropTypes.arrayOf(
PropTypes.arrayOf(
PropTypes.shape({
id: PropTypes.number.isRequired,
value: PropTypes.string.isRequired
value: PropTypes.any.isRequired,

Choose a reason for hiding this comment

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

Good job updating prop types! A more specific type you may have considered here is PropTypes.element.isRequired, as you're passing an img element.

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

Choose a reason for hiding this comment

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

Nice use of anonymous function.

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