Skip to content
This repository was archived by the owner on Jan 4, 2018. It is now read-only.

Fix for binding props and context #21

Closed
wants to merge 1 commit into from
Closed

Fix for binding props and context #21

wants to merge 1 commit into from

Conversation

vincent-dm
Copy link

No description provided.

@emmenko
Copy link
Owner

emmenko commented Aug 10, 2015

Thanks but the binding is already done by ::.

See https://github.com/zenparsing/es-function-bind

Is it not working?

@emmenko emmenko closed this Aug 10, 2015
@vincent-dm
Copy link
Author

My bad... I removed the :: when adopting your code to my ESLint standards, as it was marked as an error. Wasn't aware that I caused the problem this way.

Any reason why you don't use the connect() method to bind store and router when initializing the Application component? (as in the Redux todo-example).

Thanks for the great skeleton project by the way!

@vincent-dm vincent-dm deleted the patch-1 branch August 10, 2015 11:55
@emmenko
Copy link
Owner

emmenko commented Aug 10, 2015

Ok :)

Thanks for the great skeleton project by the way!

Glad it helps! ;)

Any reason why you don't use the connect() method to bind store and router when initializing the Application component

Hmm, not sure I understand what you mean...

@vincent-dm
Copy link
Author

I'm very new to Redux/React, so it is probably something I'm missing. But I am trying to reconcile the differences between your app and the examples on the Redux 1.0 documentation.

In your Login component the store and router are part of this.context.

In the Redux todo-example, they are bound to this.props. (See containers/App.js at http://gaearon.github.io/redux/docs/basics/ExampleTodoList.html)

@vincent-dm
Copy link
Author

Also, any reason why you use a constructor to bind this in lib/components/github/Explore.js and not the :: syntax?

I understand that this branch is not really published and it's logical that there are inconsistencies. But as a beginner it is hard to see which variations are due to "work in progress" and which ones have an underlying reason...

@emmenko
Copy link
Owner

emmenko commented Aug 10, 2015

Good points, will try to answer them tonight (currently quite busy with work) :)

Thanks!

@vincent-dm
Copy link
Author

great; looking forward to it!

@emmenko
Copy link
Owner

emmenko commented Aug 11, 2015

In your Login component the store and router are part of this.context.

Well, since I have to transition I need the router, which can only be accessed via context. About the store I access it also via context because it's much easier to get the dispatch method. Otherwise I would have to connect the Login component and get dispatch as a prop, but in the login case it's not really necessary (at least as it is now).
This might also change with the new upcoming API for connect.

In general, try to avoid using context if it's not really necessary.

Also, any reason why you use a constructor to bind this in lib/components/github/Explore.js and not the :: syntax?

You're right, it's not consistent.

But as a beginner it is hard to see which variations are due to "work in progress" and which ones have an underlying reason...

I know, I actually wanted to merge the login branch (need some more cleanup) and add JSDoc or comments to explain some of the choices. Unfortunately I'm extremely busy with work atm, I hope to find some time soon.

Hope this answer your question, otherwise feel free to ask more :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants