-
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
C16 - Spruce - Vange Spracklin #70
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.
Great work Vange! You met all the learnings goals and passed the tests for this project.
I added ways to refactor and other approaches in your PR.
Keep up the great work!
Project Grade: 🟢
let nextPlayer = (currentPlayer === PLAYER_1) ? PLAYER_2 : PLAYER_1; | ||
setCurrentPlayer(nextPlayer); |
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 ternary!
if (position.id != squareID){ | ||
return position; | ||
} | ||
if (position.value !== ''){ | ||
return position; | ||
} |
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.
Because both conditionals have the same logic when evaluated to true, we can combine them into one using the ||
(or) operator:
if (position.id != squareID){ | |
return position; | |
} | |
if (position.value !== ''){ | |
return position; | |
} | |
if (position.id != squareID || position.value !== '' ) { | |
return position; | |
} | |
return position; | ||
} | ||
madeMove = true; | ||
const newSquare = {...position, value: currentPlayer}; |
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 use of the spread operator to copy the key-value pairs and assign value
to the current player!
<h1>React Tic Tac Toe</h1> | ||
<h2>The winner is ... -- Fill in for wave 3 </h2> | ||
<button>Reset Game</button> | ||
<h2> {showStatus()} </h2> |
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 helper function! Another approach to displaying winner text is by using a ternary operator:
<h2> {showStatus()} </h2> | |
<h2> { winner ? Winner is ${winner} : `Your turn ${currentPlayer}` } </h2> |
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> | ||
} | ||
const handleOnClickSquareButton = () => { | ||
// if (props.value){ | ||
// console.log('you cant play that square!'); | ||
// }else{ | ||
props.onClickCallback(props.id); | ||
// } | ||
}; | ||
|
||
return <button | ||
className='square' | ||
onClick={handleOnClickSquareButton} | ||
> | ||
{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 could use object destructuring to reference props by their key name instead.
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> | |
} | |
const handleOnClickSquareButton = () => { | |
// if (props.value){ | |
// console.log('you cant play that square!'); | |
// }else{ | |
props.onClickCallback(props.id); | |
// } | |
}; | |
return <button | |
className='square' | |
onClick={handleOnClickSquareButton} | |
> | |
{props.value} | |
</button>; | |
}; | |
const Square = ( {value, id, onClickCallback, key} ) => { | |
const handleOnClickSquareButton = () => { | |
onClickCallback(id); | |
}; | |
return <button | |
className='square' | |
onClick={handleOnClickSquareButton} | |
> | |
{value} | |
</button>; | |
}; | |
"plugin:react-hooks/recommended" | ||
// "plugin:testing-library/react" |
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"
completed wave 1!