Conversation
ipc103
left a comment
ipc103
left a comment
There was a problem hiding this comment.
Hi @Seal125 - really nice work on this one! I left a few comments inline - the biggest things to look at would be to double check when the history should be reset, and also potentially think about letting the context do more of the heavy lifting for you to take some of the logic out of the component. Let me know if you have any questions!
| import BoardContext from './contexts/BoardContext'; | ||
|
|
||
| const Game = () => { | ||
| const { turn } = useContext(BoardContext); |
There was a problem hiding this comment.
This is cool - using the context means each component can pull exactly the data it needs. Nice work!
| @@ -0,0 +1,7 @@ | |||
| import React from 'react'; | |||
|
|
|||
| const Square = (props) => { | |||
There was a problem hiding this comment.
One small thing - you can use object destructuring if you like to avoid having to repeat props. on the next line. {clickEvent, value}
| import React from 'react'; | ||
|
|
||
| const Square = (props) => { | ||
| return <button key={props.index} onClick={props.clickEvent} className='square'>{props.value}</button>; |
There was a problem hiding this comment.
One tiny thing - the key prop should not be needed here. This is only important for React when you're rendering an array of items so it can keep track of which is which i.e. {board.map(value => >Square key={value} />} Basically, it's important to have it on the array side but not as needed here.
| <ol>{playerMoves}</ol> | ||
| </div> | ||
| <div className='row'> | ||
| <Square clickEvent={() => handleClick(0)} value={boardState[0]}/> |
There was a problem hiding this comment.
I'd probably use board.slice to take the first three items, then map over them for this row. You could then do the same thing for the row below and the second three items. Another option would be to use something like the chunk method from lodash that you could iterate over, render a row, and then render a square for each sub element.
| function stepTo(step) { | ||
| const boardInHistory = [...history]; | ||
| setBoardState(boardInHistory[step - 1] || []); | ||
| setHistory(boardInHistory[step - 1] || []); |
There was a problem hiding this comment.
It looks like we are re-setting the history each time we step backwards, so there is no way to step back forward again in the history. This means that this functions more like an undo function instead of a time-travel. For time-travel, you'd just want to not reset the history when moving backwards.
| } | ||
|
|
||
| const playerMoves = history.map((step, move) => { | ||
| const desc = move ? |
There was a problem hiding this comment.
👍 on configuring this copy - stuff like this can be a bit of a pain. Also, great work on usingmap here to easily render the elements!
| status = `The winner is ${winner}!`; | ||
| } | ||
|
|
||
| function handleClick(num) { |
There was a problem hiding this comment.
I think some of this logic might make more sense to live on the context instead of on the component. That way, the context would define not just the basic state but also an interface for updating the state. Totally up to you though - this is also a valid implementation, just a little bit different!
| const updatedHistory = [...history]; | ||
| const updatedBoard = [...boardState]; | ||
| updatedBoard[num] = turn; | ||
| updatedHistory.push(updatedBoard); |
There was a problem hiding this comment.
Another option for storing the history is to store a collection of the turns taken as opposed to the entire board. Something like [{turn: 1, move: 'X', square: 4}, {turn: 2, move: 'O', square: 0}] or something like that. That way, you don't have to track the entire board - sometimes that can be a little bit easier to reason about.
3 participants
No description provided.