Skip to content

Large update to tutorial.md's refactor section. #8795

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.

Sign up for GitHub

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

Closed
Jwan622 wants to merge 1 commit into from
Closed

Large update to tutorial.md's refactor section. #8795

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
192 changes: 154 additions & 38 deletions docs/tutorial/tutorial.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,63 +355,179 @@ class Game extends React.Component {
xIsNext: true
};
}

...
}
```

Then remove the constructor from `Board` and change `Board` so that it takes `squares` via props and has its own `onClick` prop specified by `Game`, like the transformation we made for `Square` earlier. You can pass the location of each square into the click handler so that we still know which square was clicked:
As you can see, we're pulling up `xIsNext` from the `Board` and into `Game`. Next, entirely remove the constructor and `handleClick` from `Board`. Next, change `Board` so that it takes `squares` via props that will soon be passed down from `Game`. Additionally, let's change the `renderSquare` function so that we're passing the location of each square into the passed down `onClick` handler so that we still know which square was clicked:
Copy link
Contributor

@lacker lacker Feb 2, 2017

Choose a reason for hiding this comment

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

I think I prefer the original text better here. You don't really want to walk people through all the details again, you want to explain more generally and expect that this time people understand it. So it doesn't feel right to say "remove handleClick now" - the goal of the tutorial is that when you say, this component needs an onClick prop like the other one, the student understands how to do that. If they don't then the previous section failed. It might be easier to follow along if you just hand-hold the entire way, but it doesn't necessarily make the tutorial better.

Copy link

@spences10 spences10 Mar 31, 2017

Choose a reason for hiding this comment

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

I'm struggling on this bit, it is confusing, people need to be walked through this again, it breaks the flow of the tutorial as in you have to go back to where it was mentioned previously.

I mean I've had to log an issue and then go digging through other issues and PR's to see if there's a solution to it, apologies I should have done that the other way round, but I was pretty frustrated at that point with how I couldn't make out what the documentation was instructing me to do.


```javascript
return <Square value={this.props.squares[i]} onClick={() => this.props.onClick(i)} />;
renderSquare(i) {
return <Square value={this.props.squares[i]} onClick={() => this.props.onClick(i)} />;
}
```

Lastly, let's get rid of the status logic and the status display from the `Board`'s `render` function. That will be handled by the `Game` class. The `Board`'s `render` should look like:
Copy link
Contributor

@lacker lacker Feb 2, 2017

Choose a reason for hiding this comment

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

This seems like too much. How about just adding like one sentence somewhere when the student is told to put the status logic elsewhere, that you should also remove it from Board?


```javascript
render() {
return (
<div>
<div className="board-row">
{this.renderSquare(0)}
{this.renderSquare(1)}
{this.renderSquare(2)}

...
```

`Game`'s `render` should look at the most recent history entry and can take over calculating the game status:

```javascript
const history = this.state.history;
const current = history[history.length - 1];
const winner = calculateWinner(current.squares);

let status;
if (winner) {
status = 'Winner: ' + winner;
} else {
status = 'Next player: ' + (this.state.xIsNext ? 'X' : 'O');
}
...
<div className="game-board">
<Board
squares={current.squares}
onClick={(i) => this.handleClick(i)}
/>
</div>
<div className="game-info">
<div>{status}</div>
<ol>{/* TODO */}</ol>
</div>
render() {
Copy link
Contributor

@lacker lacker Feb 2, 2017

Choose a reason for hiding this comment

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

This change seems good

const history = this.state.history;
const current = history[history.length - 1];
const winner = calculateWinner(current.squares);

let status;
if (winner) {
status = 'Winner: ' + winner;
} else {
status = 'Next player: ' + (this.state.xIsNext ? 'X' : 'O');
}

...

return (
<div className="game">
<div className="game-board">
<Board
squares={current.squares}
onClick={(i) => this.handleClick(i)}
/>
</div>
<div className="game-info">
<div>{status}</div>
<ol>{/* TODO */}</ol>
</div>
</div>
);
```

Its `handleClick` can push a new entry onto the stack by concatenating the new history entry to make a new history array:
Lastly, add a `handleClick` function to the Game class which can push a new entry onto the stack by concatenating the new history entry to make a new history array:
Copy link
Contributor

@lacker lacker Feb 2, 2017

Choose a reason for hiding this comment

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

Minor: you say "Lastly" a lot but logically only one thing can be lastly. Probably better to not do it


```javascript
handleClick(i) {
const history = this.state.history;
const current = history[history.length - 1];
const squares = current.squares.slice();
if (calculateWinner(squares) || squares[i]) {
return;
handleClick(i) {
const history = this.state.history;
const current = history[history.length - 1];
const squares = current.squares.slice();
if (calculateWinner(squares) || squares[i]) {
return;
}
squares[i] = this.state.xIsNext ? 'X' : 'O';
this.setState({
history: history.concat([{
squares: squares
}]),
xIsNext: !this.state.xIsNext,
});
}
```

At this point, Board only should have `renderSquare` and `render`; the state initialization and click handler should both live in Game.

The `Board` component should look like:

```javascript
class Board extends React.Component {
renderSquare(i) {
return <Square value={this.props.squares[i]} onClick={() => this.props.onClick(i)} />;
}

render() {
return (
<div>
<div className="board-row">
{this.renderSquare(0)}
{this.renderSquare(1)}
{this.renderSquare(2)}
</div>
<div className="board-row">
{this.renderSquare(3)}
{this.renderSquare(4)}
{this.renderSquare(5)}
</div>
<div className="board-row">
{this.renderSquare(6)}
{this.renderSquare(7)}
{this.renderSquare(8)}
</div>
</div>
);
}
squares[i] = this.state.xIsNext ? 'X' : 'O';
this.setState({
history: history.concat([{
squares: squares
}]),
xIsNext: !this.state.xIsNext,
});
}
```

At this point, Board only needs `renderSquare` and `render`; the state initialization and click handler should both live in Game.
while the `Game` component should look like this:
Copy link
Contributor

@lacker lacker Feb 2, 2017

Choose a reason for hiding this comment

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

Maybe rather than splatting all the code inline, it would be better just to refer to the codepen where the solution code is - I found that more helpful, to just have the correct answer entirely. But I think we already do that?


```javascript
class Game extends React.Component {
constructor() {
super();
this.state = {
history: [{
squares: Array(9).fill(null)
}],
xIsNext: true
};
}

handleClick(i) {
const history = this.state.history;
const current = history[history.length - 1];
const squares = current.squares.slice();
if (calculateWinner(squares) || squares[i]) {
return;
}
squares[i] = this.state.xIsNext ? 'X' : 'O';
this.setState({
history: history.concat([{
squares: squares
}]),
xIsNext: !this.state.xIsNext,
});
}

render() {
const history = this.state.history;
const current = history[history.length - 1];
const winner = calculateWinner(current.squares);

let status;
if (winner) {
status = 'Winner: ' + winner;
} else {
status = 'Next player: ' + (this.state.xIsNext ? 'X' : 'O');
}

return (
<div className="game">
<div className="game-board">
<Board
squares={current.squares}
onClick={(i) => this.handleClick(i)}
/>
</div>
<div className="game-info">
<div>{status}</div>
<ol>{/* TODO */}</ol>
</div>
</div>
);
}
}
```

## Showing the Moves

Expand Down