-
Notifications
You must be signed in to change notification settings - Fork 80
Asya Spruciiiee #76
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
base: main
Are you sure you want to change the base?
Asya Spruciiiee #76
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.
Good work Asya! Your logic to mark a square works. We're just missing the logic to check for winner which is just one more function.
I have added comments on ways to refactor your project as it currently is.
Let me know if you'd like to work on the rest of the project together!
Grade: 🟡 Yellow
"plugin:react/recommended", | ||
"plugin:jsx-a11y/recommended", | ||
"plugin:react-hooks/recommended", | ||
"plugin:jest/recommended", |
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.
I assume jest
was removed from VS Code because it was running tests with every change right?
We actually turn off auto run by adding the key-value pair to our settings.json
in VS Code:
"jest.autoRun": "off"
src/App.js
Outdated
// Then pass it into the squares as a callback | ||
|
||
const clickOnSquares = (squareId) => { | ||
console.log('Is this thing working'); |
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.
When pushing code to be reviewed, we should eliminate any print statements or console logs
console.log('Is this thing working'); |
src/App.js
Outdated
|
||
const clickOnSquares = (squareId) => { | ||
console.log('Is this thing working'); | ||
let newSquares = [...squares]; |
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.
Great job in making a copy of the squares data! Here's a reminder on why we need to make the copy:
To trigger the update
stage for this component, React needs to notice a change in state data. We could mutate state directly, however, this doesn't scale well and doesn't follow React best practices.
Instead, we should pass in a copy of our state data to the setState function setSquares()
which will have React compare the object references between the copy and the old state data. If the object references are different, React will then trigger the update
stage.
src/App.js
Outdated
const clickOnSquares = (squareId) => { | ||
console.log('Is this thing working'); | ||
let newSquares = [...squares]; | ||
console.log(newSquares); |
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.
console.log(newSquares); |
src/App.js
Outdated
for (let row = 0; row < newSquares.length; row++) { | ||
for (let columns = 0; columns < newSquares.length; columns++) { | ||
if (newSquares[row][columns].id === squareId) { | ||
newSquares[row][columns].value = 'X'; | ||
} | ||
} | ||
} |
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.
We also want to make sure we mark squares that haven't been marked yet. We can do so by checking if the value of that square is empty.
for (let row = 0; row < newSquares.length; row++) { | |
for (let columns = 0; columns < newSquares.length; columns++) { | |
if (newSquares[row][columns].id === squareId) { | |
newSquares[row][columns].value = 'X'; | |
} | |
} | |
} | |
for (let row = 0; row < newSquares.length; row++) { | |
for (let columns = 0; columns < newSquares.length; columns++) { | |
if (newSquares[row][columns].id === squareId && newSquares[row][columns].value === '') { | |
newSquares[row][columns].value = 'X'; | |
} | |
} | |
} |
div.grid { | ||
width: 600px; | ||
height: 600px; | ||
margin: 0 auto; | ||
background-color: #34495e; | ||
color: #fff; | ||
border: 6px solid #2c3e50; | ||
border-radius: 10px; | ||
display: grid; | ||
grid-template: repeat(3, 1fr) / repeat(3, 1fr); | ||
width: 600px; | ||
height: 600px; | ||
margin: 0 auto; | ||
background-color: #ffc2cc; | ||
color: #fff; | ||
border: 6px solid #ffc2cc; | ||
border-radius: 10px; | ||
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.
Love the pink css additions!
const Square = (props) => { | ||
// For Wave 1 enable this | ||
// Component to alert a parent | ||
// component when it's clicked on. | ||
|
||
return <button | ||
className="square" | ||
> | ||
{props.value} | ||
</button> | ||
} | ||
return <button className='square' onClick={() => props.onClickCallback(props.id)}>{props.value}</button>; | ||
}; |
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.
We can use object destructuring to reference the props data by their key name instead of using dot notation:
const Square = ( {value, id, onClickCallback, key} ) => {
return <button className='square' onClick={() => onClickCallback(id)}>{value}</button>;
};
}
Also, good work in using an anonymous callback function for the button's onClick
.
…le to stop click functionality when the game has a winner
IM FINALLY DONE(PARITALLY)