Large update to tutorial.md's refactor section. #8795
Conversation
So I think this section could use some fleshing out. There simply isn't enough instruction for beginners to follow along and even worse, the removal of status logic from `Board` was never completed, resulting in duplicate statuses in the final code. This clarifies some of the major talking points in this section and it fixes the duplicate logic problem in the `Board` class.
Jwan622
changed the title
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks! If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact [email protected] if you have any questions. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
| ``` | ||
| 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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| } | ||
| ``` | ||
| 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: |
There was a problem hiding this comment.
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?
| ``` | ||
| 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: |
There was a problem hiding this comment.
Minor: you say "Lastly" a lot but logically only one thing can be lastly. Probably better to not do it
| <div>{status}</div> | ||
| <ol>{/* TODO */}</ol> | ||
| </div> | ||
| render() { |
| ``` | ||
| 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: |
There was a problem hiding this comment.
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?
|
This is kind of tough because it's very stylistic how much detail this tutorial should go into. I tried to provide details of where I thought the original was better, but I feel like you might kinda fundamentally disagree on it but let me know what you think. |
sebmarkbage
left a comment
sebmarkbage
left a comment
There was a problem hiding this comment.
Changes requested above.
|
Hey guys! Sorry to be so late to the responses... I will probably have time to look at them this week if you don't think the changes I made are totally up to par. Thoughts? Merge it now or have me re-review your comments? In short, I feel like from a pedagogical standpoint, I probably philosophically add more color than the original authors, but that's simply because I feel like repeating things and adding logical connection is useful for beginners while, for more advanced readers, it's annoying or not helpful. But that's where I side. I'm happy to reread some of the suggestions you made, but be warned, I've lost context and it may take me some more time to re-read the comments. |
|
I found this really helpful @Jwan622, thank you 👍 |
|
cc @brigand |
|
Hey, sorry we didn’t iterate further on this. I sort of took ownership of the tutorial, and I agree it wasn’t detailed enough. It was written with Facebook employees in mind originally, so of course it needed to be more detailed for the outside community. @brigand and I rewrote some parts of the tutorial in #9454, and following along should be easier now. https://facebook.github.io/react/tutorial/tutorial.html Again, I appreciate the effort that went into this. |
|
You guys are the best!
…On Mon, May 1, 2017 at 12:41 PM, Dan Abramov ***@***.***> wrote:
Hey, sorry we didn’t iterate further on this.
I sort of took ownership of the tutorial, and I agree it wasn’t detailed
enough. It was written with Facebook employees in mind originally, so of
course it needed to be more detailed for the outside community.
@brigand <https://github.com/brigand> and I rewrote some parts of the
tutorial in #9454 <#9454>, and
following along should be easier now.
https://facebook.github.io/react/tutorial/tutorial.html
Again, I appreciate the effort that went into this.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8795 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AGecOZMWtPs3qHcYRfC-7AJFzHMT6Spuks5r1gtGgaJpZM4Lju0G>
.
--
Jeffrey Wan
Blue Apron Engineering
40 W 23rd St., 5th floor
New York, NY 10010
|
6 participants
So I think this section could use some fleshing out. I think this was the hardest section to follow. There simply isn't enough instruction for beginners to follow along and even worse, the removal of status logic from
Boardwas never completed, resulting in duplicate statuses in the final code. This clarifies some of the major talking points in this section and it fixes the duplicate logic problem in theBoardclass.Lastly, it adds a state of code snapshot at the end of this refactor section to help students who may have broken their code along the way.
Before submitting a pull request, please make sure the following is done...
master.npm test).npm run lint) - we've done our best to make sure these rules match our internal linting guidelines.