Skip to content

Update "Usage with React" #1285

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

Merged
merged 26 commits into from
Feb 2, 2016
Merged

Update "Usage with React" #1285

merged 26 commits into from
Feb 2, 2016

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Jan 26, 2016

This builds on top of #1186 with some changes I wanted to make to the wording and structure. "Usage with React" looks done to me (although improvements are welcome!)

Right now I really want somebody to contribute a few things here:

Help greatly appreciated because I'm busy but I really really want to ship this.
Please post in this thread if you'd like to take some part of this work.

@gaearon
Copy link
Contributor Author

gaearon commented Jan 26, 2016

cc @omnidan who owns todos-with-undo, maybe you've got some time to help?

@pedrottimark
Copy link
Contributor

I will go for the first bullet point: basics/ExampleTodoList.md

@gaearon
Copy link
Contributor Author

gaearon commented Jan 26, 2016

To make it super clear, the new "Usage with React" is here:
https://github.com/rackt/redux/blob/___react-docs-update/docs/basics/UsageWithReact.md

@badnorseman
Copy link
Contributor

@gaearon @pedrottimark Perhaps this can be helpful. It is the source code used for the update to Usage with React. I didn't include tests, otherwise the code is complete.

@pedrottimark
Copy link
Contributor

@urbanvikingr Do you think it would be more efficient and less risk of error for you to do it? No offense taken, if you have time and want to.

@gaearon
Copy link
Contributor Author

gaearon commented Jan 26, 2016

Note there were mixed code styles in previous PR. I tried to amend those to match our code style enforced by the linter.

@pedrottimark
Copy link
Contributor

@gaearon So the goal is on ___react-docs-update branch: copy each chunk of code from UsageWithReact.md paste it where it goes in the order of ExampleTodoList.md but leave all other code there which is not in the intersection unchanged (for example, Action Creators and Constants).

@pedrottimark
Copy link
Contributor

@gaearon Hopefully the Pooh Bear brain of a hybrid writer-designer-developer doesn’t drive you crazy.

  1. In Usage, the Presentational Components have two different orders.
    It might help smooth things out if the order under Implementing Components changes to become consistent with the order previously established under Designing Component Hierarchy: TodoList, Todo, Link, Footer, App.
    If you agree, should I include that in the pull request?
  2. What do you think if the major and minor order of source for the components in the lower half of Examples becomes parallel with Usage?
    That is, Presentational Components, Container Components, Other Components
    Specific minor order depends on answer to preceding question.

@gaearon
Copy link
Contributor Author

gaearon commented Jan 27, 2016

So the goal is on ___react-docs-update branch: copy each chunk of code from UsageWithReact.md paste it where it goes in the order of ExampleTodoList.md but leave all other code there which is not in the intersection unchanged (for example, Action Creators and Constants).

For the rest of the code, use code from https://github.com/urbanvikingr/todo/. It's based on my Egghead tutorials.

It might help smooth things out if the order under Implementing Components changes to become consistent with the order previously established under Designing Component Hierarchy: TodoList, Todo, Link, Footer, App.

I agree order should be consistent. We should strive to maintain “first declared, then used” order. So Todo should be before TodoList.

@gaearon
Copy link
Contributor Author

gaearon commented Jan 27, 2016

What do you think if the major and minor order of source for the components in the lower half of Examples becomes parallel with Usage?

Yes, order everywhere should be consistent. Thanks for helping!

@pedrottimark
Copy link
Contributor

@gaearon Thank you as always for your patience to explain. Super, I will use “first declared, then used” order as the guiding principle when there is an inconsistency. Am keeping notes as other little things come up to ask as a batch tomorrow.

@gaearon
Copy link
Contributor Author

gaearon commented Jan 27, 2016

👍

@gaearon
Copy link
Contributor Author

gaearon commented Jan 27, 2016

@pedrottimark

I gave you the commit bit so please feel free to edit the branch right as part of this PR. I'd like if you could flush your temporary progress so we have some shared bits others could use for the rest of the bullet points.

@badnorseman
Copy link
Contributor

@pedrottimark I can update source code for ToDo this weekend.

@jakegardner
Copy link
Contributor

I'll take the second bullet: recipes/ComputingDerivedData.md

@gaearon
Copy link
Contributor Author

gaearon commented Jan 27, 2016

@jakegardner Thank you!

@gaearon
Copy link
Contributor Author

gaearon commented Jan 28, 2016

I'd imagine something like this: (pardon the renames)

import { getVisibleTodos } from '../selectors'

const mapStateToProps = (state) => {
  return {
    todos: getVisibleTodos(state)
  }
}
...
const VisibleTodoList = connect(
  mapStateToProps,
  mapDispatchToProps
)(TodoList)

I don't see the value in *selector suffix.

@jakegardner
Copy link
Contributor

I agree terse names are better, but then it would be the same as the non-memoized version. What do you suggest?

Maybe I'm missing something. The output of createSelector() is a function that returns the memoized state. In the previous version of this document, that output was mapped as props to the child components of App. Now that we're doing the filtering in VisibleTodoList without a render method, I'm not sure how to map the output of createSelector() (via visibleTodosSelector) to child props.

@gaearon
Copy link
Contributor Author

gaearon commented Jan 28, 2016

Doesn't my example show how? Call selector from mapStateToProps. I think :-)

@jakegardner
Copy link
Contributor

Ok, I was trying to not modify the selector definition. But as you wish :-)

Have a look: #1295

Update todoStore to store.
@badnorseman
Copy link
Contributor

Found a bug #1332

Update todoStore to store.
@badnorseman
Copy link
Contributor

@gaearon I'm updating todomvc example first. Already updated the source code, just need to complete tests. Check #1334 for progress.

@badnorseman
Copy link
Contributor

@gaearon I suggest that I rename todomvc example to todos. When I update the todos-with-undo example, I will use the same file structure as in the todos example. Agree?

@gaearon
Copy link
Contributor Author

gaearon commented Jan 31, 2016

The problem is that todomvc has more "stuff" than todos-with-undo. But I agree it's weird they are so different. Do as you think is best, and I'll take a look later!

@badnorseman
Copy link
Contributor

OK, I'm going to rename todomvc to todos and fix the tests. Then we can decide whether to add additional functionality or not. Right now, I think that we need to align examples with documentation, so newcomers can follow.

@badnorseman
Copy link
Contributor

Decided to keep the todomvc example and add todos. I could use some help with unit test of components and containers in the todos example.

@gaearon
Copy link
Contributor Author

gaearon commented Feb 2, 2016

@urbanvikingr Where can I find the latest version you were working on?

@badnorseman
Copy link
Contributor

bug in doc #1332
todos #1334 (just saw that it now has merge conflicts)

Perhaps we can merge doc updates?

Bug in "Usage With React" and "Example Todo List" docs
@gaearon
Copy link
Contributor Author

gaearon commented Feb 2, 2016

@urbanvikingr I’ll take care of merge conflicts, thanks. Let me take some time and I’ll merge todos in this PR.

@badnorseman
Copy link
Contributor

@gaearon Awesome. Thanks. Then todos are merged, I will update doc references from todomvc to todos. Perhaps this weekend, I have time to look into unit testing of components and containers.

gaearon added a commit that referenced this pull request Feb 2, 2016
@gaearon gaearon merged commit 6780691 into master Feb 2, 2016
@gaearon gaearon deleted the ___react-docs-update branch February 2, 2016 22:06
@gaearon
Copy link
Contributor Author

gaearon commented Feb 2, 2016

Thanks to everyone involved! This is up: http://redux.js.org/docs/basics/UsageWithReact.html

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.

5 participants