Skip to content

Update to Usage With React documention #1186

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

Closed
wants to merge 5 commits into from
Closed

Update to Usage With React documention #1186

wants to merge 5 commits into from

Conversation

badnorseman
Copy link
Contributor

The documentation UsageWithReact.md has been updated with code samples according to Egghead.io video series. A complete sample app can be found here https://github.com/urbanvikingr/todo. Please tell me how you would like to bring it to Rackt.

@@ -52,18 +52,16 @@ It is advisable that only top-level components of your app (such as route handle
</tbody>
</table>

In this todo app, we will only have a single container component at the top of our view hierarchy. In more complex apps, you might have several of them. While you may nest container components, we suggest that you pass props down whenever possible.
In this todo app, we will have three container components that pass props to presentational components. We suggest that you do not nest container components and that you pass props down whenever possible.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say "We suggest that you do not nest container components beyond necessity. Start by having a few top-level container components pass the props down to the presentational component tree. When you notice that components in the middle of that tree pass too many props down without actually using them, it is a good time to introduce some containers."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I have updated PR.


These are all presentational components. They don’t know *where* the data comes from, or *how* to change it. They only render what’s given to them.

If you migrate from Redux to something else, you’ll be able to keep all these components exactly the same. They have no dependency on Redux.

Let’s write them! We don’t need to think about binding to Redux yet. You can just give them fake data while you experiment until they render correctly.
We also have some container components that connects to Redux. The container components are responsible for getting and transforming state. Remember that changes result in a new state as state is immutable.
Copy link
Contributor

Choose a reason for hiding this comment

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

"that connect"

Copy link
Contributor

Choose a reason for hiding this comment

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

I think "getting and transforming state" is a bit confusing. Changes happen in reducer, so I'm not sure why the next sentence is relevant here.

Maybe something like "Container components calculate the props to pass to the presentational components they wrap based on the current state of the Redux store." could work better instead.

@gaearon
Copy link
Contributor

gaearon commented Jan 13, 2016

Thanks for your work on this! I left some comments. Also, todos-with-undo example needs updating, as well as the Implementing Undo History and Computing Derived Data recipes, as they contain fragments of the same code.

@badnorseman
Copy link
Contributor Author

@gaearon Thanks, I'll update your comments this weekend. I also have the source code to match. Would you have time to review https://github.com/urbanvikingr/todo. Would you it to be transferred to this repo?

@gaearon
Copy link
Contributor

gaearon commented Jan 26, 2016

Thank you very much. I continued your work in #1285.

@gaearon gaearon closed this Jan 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants