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

C16 - Spruce - Vange Spracklin #70

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

Conversation

HouseOfVange
Copy link

completed wave 1!

Copy link

@audreyandoy audreyandoy left a 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: 🟢

Comment on lines +63 to +64
let nextPlayer = (currentPlayer === PLAYER_1) ? PLAYER_2 : PLAYER_1;
setCurrentPlayer(nextPlayer);

Choose a reason for hiding this comment

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

Nice ternary!

Comment on lines +47 to +52
if (position.id != squareID){
return position;
}
if (position.value !== ''){
return position;
}

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:

Suggested change
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};

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>

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:

Suggested change
<h2> {showStatus()} </h2>
<h2> { winner ? Winner is ${winner} : `Your turn ${currentPlayer}` } </h2>

Comment on lines 6 to +25
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>;
};
Copy link

@audreyandoy audreyandoy Jan 12, 2022

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.

Suggested change
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>;
};

Comment on lines +14 to +15
"plugin:react-hooks/recommended"
// "plugin:testing-library/react"
Copy link

@audreyandoy audreyandoy Jan 12, 2022

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"

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