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

Peter's Submission #6

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

PeterTheDeveloper
Copy link

No description provided.

Copy link

@ipc103 ipc103 left a comment

Choose a reason for hiding this comment

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

Hey @PeterTheDeveloper - nice work on this one! I wasn't able to boot the app up locally for some reason (seems like maybe the package.json file was missing from the diff? No worries though - the code looks really good overall. I like the choice to use the context to help hold some of your game state. One thought I had is that you could potentially put even more of the logic onto the context and let your components be a little bit more presentational. It's up to you though - there are definitely some downsides to that approach too! Feel free to respond to any of the comments or on the thread here if you have any questions!

}
return null;
}
threeInARow()
Copy link

Choose a reason for hiding this comment

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

I think you need a return statement here to either return null or the winner.

<h3>{moves}</h3>

<div className="row board-row">
<Square onClick={() => clickHandler(0)} value={board[0]} />
Copy link

Choose a reason for hiding this comment

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

You can shorten this up a bit by using the chunk method from lodash if you like, or by implementing your own chunk method! https://lodash.com/docs/4.17.15#chunk

{_.chunk(board, 3).map(row => (
  <div className="row board-row">{row.map((cell, i) => <Square onClick={() => clickHandler(i)} value={cell} }</div>
))}

@@ -0,0 +1,9 @@
import React from 'react'

function Square(props) {
Copy link

Choose a reason for hiding this comment

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

One small note: you can use object destructuring if you like to avoid needing to call props. multiple times on the next line. function Square({click, value})

setBoard(boardInHistory[turn]);
}

function clickHandler(i) {
Copy link

Choose a reason for hiding this comment

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

No right or wrong answer here, but I think you could make an argument that this logic could fit nicely on the context. Instead of the context providing setBoard, setHistory, and setTurn, you could just pass this function and let it get called as the callback. The downside if your context becomes less flexible, but the upside is that the component stays shorter.

Copy link

Choose a reason for hiding this comment

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

One other upside to that approach is that the Square component could pull it's callback directly from the context instead of requiring it as a prop.

@ipc103 ipc103 removed their assignment Apr 19, 2021
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