1260864 - Add docs on getting data from Redux stores

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect)

Comment 1

[Lin Clark [:linclark]]

Attachment: Bug1260864-redux-docs.patch

Happy to discuss and change. It has a few implicit recommendations in there, so lmk if you have any questions.

Comment 2

[James Long (:jlongster)]

Comment on attachment 8736444 [details] [diff] [review]
Bug1260864-redux-docs.patch

Review of attachment 8736444 [details] [diff] [review]:
-----------------------------------------------------------------

This looks generally good to me, a few comments:

1. For simple connections, I like just specifying the selector inline:

connect(
  state => ({ items: state.items })
)(Component);

I don't think we should be too prescriptive about this, so you could make a quick not that you can do simple selectors inline? Unless you think we should be consistent, that's fine.

2. It would be good to make a quick note that the selector also takes `props` as the second argument if you need it. That's good for parameterizing the connection.

3. Might also be good to note that the second argument to `connect` allows you to hook up action creators.

4. Add a link to the react-redux docs about connect

For #2 and #3, you can be a light on details as you want. Maybe even just one sentence saying those things are possible, and refer them to the react-redux docs to see the docs.

Comment 3

[Lin Clark [:linclark]]

Attachment: Bug1260864-redux-docs.patch

Thanks for the review!

I think the fact that you can specify a selector in line can be left implicit and up to the discretion of the coder and reviewer. If we make it explicit here, then in other docs for things you can do either way we'd also have to make it explicit.

Good idea to add the other parameters. I've included the note.

Comment 4

[James Long (:jlongster)]

Comment on attachment 8737203 [details] [diff] [review]
Bug1260864-redux-docs.patch

Review of attachment 8737203 [details] [diff] [review]:
-----------------------------------------------------------------

Sure, but people will end up just copying what's in the docs (re `mapStateToProps`). Here it's not really needed for such a simple selector. I don't think we need to explain it everywhere but if we're documenting `connect` it's the place to put it.

It's not a big deal though. These docs will change as we write more, and there's obviously a bunch more redux-specific things to write so this will evolve. This is good as is and you can go ahead and land it.

Comment 5

[Lin Clark [:linclark]]

Sounds good, thanks for the review.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/67bf00c5ecda

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/67bf00c5ecda