-
Notifications
You must be signed in to change notification settings - Fork 45.7k
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
[Tutorial] Make it easier to follow the instructions #9454
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.
What about adding "context" with class or function declaration to all other places? So that it's more obvious where we are changing.
@gaearon am I on the right track? |
I think this is ready for review. |
docs/tutorial/tutorial.md
Outdated
class Square extends React.Component { | ||
constructor() { | ||
super(); | ||
this.state = { | ||
value: null, | ||
}; | ||
} | ||
// ... | ||
render() { | ||
// same as above |
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.
Maybe just copy and paste it here? With highlighting it's not really that distracting, and it helps to stay in the flow.
This is a good start. Do you want to take a pass at the whole tutorial and do the same thing anywhere there is not quite enough context? For example I’m thinking of later I’m not sure if it’s better to copy and paste or leave a comment like “// same as before”—I’d leave this to your discretion (whatever is easier to follow in every particular case). |
docs/tutorial/tutorial.md
Outdated
@@ -132,7 +132,7 @@ After: You should see a number in each square in the rendered output. | |||
|
|||
### An Interactive Component | |||
|
|||
Let's make the Square component fill in an "X" when you click it. Try changing the button tag returned in the `render()` function of the `Square` like this: |
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.
There was inconsistent use of backticks in component names. Removing them reduces some noise.
docs/tutorial/tutorial.md
Outdated
<div> | ||
<div className="status">{status}</div> | ||
<div className="board-row"> | ||
{this.renderSquare(0)}{this.renderSquare(1)}{this.renderSquare(2)} |
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.
Just saving some vertical space by putting these on one line.
I think that in the cases where we're replacing a previously defined method, we don't need to show the full class. Happy to change that if you disagree. Edit: specifically talking about Board and Game which get quite large. |
docs/tutorial/tutorial.md
Outdated
@@ -200,6 +226,10 @@ class Board extends React.Component { | |||
squares: Array(9).fill(null), | |||
}; | |||
} | |||
render() { | |||
// TODO |
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.
This is not what I see when I follow along from the starter CodePen. Can we either make it similar or make it clear the code is just being skipped?
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.
Fixed.
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.
This looks great. Let’s just make sure all CodePens are working. It would help if you could do a complete read-through and check for mistakes.
docs/tutorial/tutorial.md
Outdated
``` | ||
|
||
Whenever `this.setState` is called, an update to the component is scheduled, causing React to merge in the passed state update and rerender the component along with its descendants. When the component rerenders, `this.state.value` will be `'X'` so you'll see an X in the grid. | ||
|
||
If you click on any square, an X should show up in it. | ||
|
||
<a href="https://codepen.io/brigand/pen/XRjPRg?editors=0010" target="_blank">View the current code</a>. |
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.
"If you click on any square, an X should show up in it."
I don't think the linked CodePen does this. I see an alert in it.
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.
Fixed.
docs/tutorial/tutorial.md
Outdated
<div> | ||
<div className="status">{status}</div> | ||
<div className="board-row"> | ||
{this.renderSquare(0)}{this.renderSquare(1)}{this.renderSquare(2)} |
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’m okay with this, but please change the existing “final” CodePen linked from intro paragraphs to match the new style. They should all be consistent with the doc.
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.
Also I think this might give people the wrong impression that JSX requires you put things on a single line or they’ll be split by newlines. This is not true. So maybe wordier version is clearer in this case.
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.
Fixed, went with the multiple lines. I'll update the codepens next.
docs/tutorial/tutorial.md
Outdated
@@ -315,19 +426,21 @@ function Square(props) { | |||
|
|||
You'll need to change `this.props` to `props` both times it appears. Many components in your apps will be able to be written as functional components: these components tend to be easier to write and React will optimize them more in the future. | |||
|
|||
<a href="https://codepen.io/brigand/pen/OmRoGV?editors=0010" target="_blank">View the current code</a>. |
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.
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.
Fixed.
Not ready for review yet. |
Thanks. Yesterday I went through the tutorial (in its current form) with two people learning React, and got a lot of feedback. I will summarize it here. I would appreciate if you could dedicate some time to fixing these issues, as they are related to the work you’re doing (in fact you may already have solved some of them).
I’m sorry this is a lot of stuff, but I believe we can only address this in a batch with somebody who feels ownership over this. I’m very pleased with how you handle those issues so far so I think you’re the best person to address this comprehensively. Thanks! |
docs/tutorial/tutorial.md
Outdated
@@ -34,7 +34,7 @@ We'll be using an online editor called CodePen in this guide. You can begin by o | |||
|
|||
#### Following Along Locally | |||
|
|||
You can also follow along locally if you don't mind a few extra steps: | |||
Alternatively, you can set up a project on your computer. This is more work, but gives you a standard development environment, including support for <a href="https://medium.freecodecamp.com/javascript-modules-a-beginner-s-guide-783f7d7a5fcc" target="_blank">modules</a>. |
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 put a lot of thought into this piece, but I don't think it's quite right. My thoughts:
I think "alternatively" is a clear enough indicator that this isn't required.
"standard development environment" is presumptuous, but I think it's important to emphasize that this reflects the way React apps are developed.
Understanding modules is an important part of normal React app development, but maybe we shouldn't send readers off in that direction at this point.
I'm going to keep working on this. Sorry for not reviewing my work thoroughly last time. I'll address the new issues over the next few days. |
@gaearon ready for review. |
I made some more changes related to fixing accidentally missing pieces, rewording "replace X with Y" instructions to be clearer, and removing the inconsistent anchor tags (let's just use Markdown links everywhere). |
I am merging this but I'd appreciate another proof read now. |
* tutorial: adds note about onClick * tutorial: show full square component * merge * fixes line number * tutorial: misc changes * fixes Board render initial code sample * [tutorial] adds codepen links and misc small fixes * removes useless arrow functions, #9531 * {this.renderSquare} new lines * be more explicit about history state * fixes highlight * following along locally * changes todo to this.props.value * removes calculateWinner from initial codepens and includes it in tutorial * removes note about calculateWinner at end of file * adds debug-view and debug-view-final * removes debug view, updates codepen instructions * adds another codepen * tutorial.md * tutorial.md * tutorial.md * tutorial.md * Put . into links for consistency with docs * Make the very first change easier to follow * A few more changes (cherry picked from commit e9d6f3f)
Looks great! Sorry I wasn't able to complete my PR... was a bit busy at work. But great job! |
* tutorial: adds note about onClick * tutorial: show full square component * merge * fixes line number * tutorial: misc changes * fixes Board render initial code sample * [tutorial] adds codepen links and misc small fixes * removes useless arrow functions, #9531 * {this.renderSquare} new lines * be more explicit about history state * fixes highlight * following along locally * changes todo to this.props.value * removes calculateWinner from initial codepens and includes it in tutorial * removes note about calculateWinner at end of file * adds debug-view and debug-view-final * removes debug view, updates codepen instructions * adds another codepen * tutorial.md * tutorial.md * tutorial.md * tutorial.md * Put . into links for consistency with docs * Make the very first change easier to follow * A few more changes (cherry picked from commit e9d6f3f)
It's just a few extra lines of code to show the full
Square
component so I've included it here.Ref: #9120
Also added a note about passing a function vs calling a function. This comes up a lot on IRC; especially with people coming from angular, or with less experience with functions as values