-
Notifications
You must be signed in to change notification settings - Fork 80
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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); |
There was a problem hiding this comment.
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
.
import matthew from './img/matthewface.png'; | ||
import junior from './img/juniorface.jpg'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💜
return { | ||
c048: [], | ||
c012: [], | ||
c345: [], | ||
c678: [], | ||
c036: [], | ||
c147: [], | ||
c258: [], | ||
c642: [], | ||
}; | ||
}; |
There was a problem hiding this comment.
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.
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; | ||
}); | ||
}); |
There was a problem hiding this comment.
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} /> |
There was a problem hiding this comment.
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
display: grid; | ||
grid-template: repeat(3, 1fr) / repeat(3, 1fr); |
There was a problem hiding this comment.
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} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job remembering key
!
const PLAYER_1 = <img src={matthew} alt="X" />; | ||
const PLAYER_2 = <img src={junior} alt="O" />; |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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.
onClick={() => { | ||
updateSquares(id); | ||
}} |
There was a problem hiding this comment.
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.
No description provided.