Add docs on getting data from Redux stores

RESOLVED FIXED in Firefox 48

Status

RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: linclark, Assigned: linclark)

Tracking

Trunk
Firefox 48

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

2 years ago
Created attachment 8736444 [details] [diff] [review]
Bug1260864-redux-docs.patch

Happy to discuss and change. It has a few implicit recommendations in there, so lmk if you have any questions.
Assignee: nobody → lclark
Status: NEW → ASSIGNED
Attachment #8736444 - Flags: review?(jlong)
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.
(Assignee)

Comment 3

2 years ago
Created attachment 8737203 [details] [diff] [review]
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.
Attachment #8736444 - Attachment is obsolete: true
Attachment #8736444 - Flags: review?(jlong)
Attachment #8737203 - Flags: review?(jlong)
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.
Attachment #8737203 - Flags: review?(jlong) → review+
(Assignee)

Comment 5

2 years ago
Sounds good, thanks for the review.
Keywords: checkin-needed

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/67bf00c5ecda
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.