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

update counter example to use react-redux v0.5.1 #424

Closed
wants to merge 5 commits into
base: breaking-changes-1.0
from

Conversation

2 participants
@hartzis
Contributor

hartzis commented Aug 7, 2015

Update Counter example with most recent react-redux.

Based on reduxjs/react-redux#29.

Remove CounterApp component and instead connect to the Counter component itself.

I was also debating using the example with Object.assign, but didn't want to mess with all the different ways to bring that in. Destructuring off actions seems to work alright.

Let me know what you think.

@hartzis

This comment has been minimized.

Contributor

hartzis commented Aug 7, 2015

I missed updating the tests, working on them now.

Brian Hartz
@hartzis

This comment has been minimized.

Owner

hartzis commented on examples/counter/components/Counter.js in e2fb9a6 Aug 7, 2015

Did this to allow for explicit testing on the non-redux-connected component.

Brian Hartz
@hartzis

This comment has been minimized.

Contributor

hartzis commented Aug 8, 2015

@gaearon Just saw you updated react-redux to v0.6.0. I'll update this request in the next couple of days.

class Counter extends Component {
export class CounterComponent extends Component {

This comment has been minimized.

@gaearon

gaearon Aug 9, 2015

Contributor

Let's keep calling it Counter?

@@ -1,23 +0,0 @@
import React, { Component } from 'react';

This comment has been minimized.

@gaearon

gaearon Aug 9, 2015

Contributor

I'd do it differently. Let's encourage separation of “smart” and “dumb” components. See https://github.com/gaearon/react-redux#dumb-component-is-unaware-of-redux and https://github.com/gaearon/react-redux#smart-component-is-connect-ed-to-redux.

@hartzis

This comment has been minimized.

Contributor

hartzis commented Aug 10, 2015

@gaearon Definitely agree with the "smart" and "dumb" component separation. I think this now appears to match your react-redux readme example. Let me know what you think.

@gaearon

This comment has been minimized.

Contributor

gaearon commented Aug 10, 2015

Any specific reason you're putting them in actions instead of just spreading, like before?

@hartzis

This comment has been minimized.

Contributor

hartzis commented Aug 10, 2015

It seemed more explicit assigning actions to an actions prop, example.

I also didn't want to require an 'Object.Assign' library, but now I realize we could do this instead:

function mapDispatchToProps(dispatch) {
  return {
    ...bindActionCreators(CounterActions, dispatch)
  }
}

vs
https://github.com/gaearon/react-redux#inject-todos-and-all-todoactioncreators-and-counteractioncreators-directly-as-props

function mapDispatchToProps(dispatch) {
  return bindActionCreators(Object.assign({}, todoActionCreators, counterActionCreators), dispatch);
}

I do like the spread operator for mapDispatchToProps.

@gaearon

This comment has been minimized.

Contributor

gaearon commented Aug 10, 2015

This:

function mapDispatchToProps(dispatch) {
  return {
    ...bindActionCreators(CounterActions, dispatch)
  }
}

is exactly equivalent to this:

function mapDispatchToProps(dispatch) {
  return bindActionCreators(CounterActions, dispatch);
}

There is no need for spread here.

Brian Hartz
@hartzis

This comment has been minimized.

Contributor

hartzis commented Aug 10, 2015

Thank you sir, nice catch.

Sounds good. Fixed spreading actions into Counter.

@hartzis

This comment has been minimized.

Contributor

hartzis commented Aug 10, 2015

@gaearon Would you like me to resubmit/squash this pull request to a single commit? I realize only CounterApp and package.json should have been updated.

@gaearon

This comment has been minimized.

Contributor

gaearon commented Aug 10, 2015

Yes, please!

@hartzis hartzis closed this Aug 10, 2015

@gaearon

This comment has been minimized.

Contributor

gaearon commented Aug 10, 2015

Did you mean to open a new PR?

@hartzis

This comment has been minimized.

Contributor

hartzis commented Aug 10, 2015

I did, #453. I thought it would be easier/cleaner.

@gaearon

This comment has been minimized.

Contributor

gaearon commented Aug 10, 2015

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment