cloverfield-tools / universal-react-boilerplate

A simple boilerplate Node app.
MIT License
904 stars 97 forks source link

Feature/react router with redux #56

Closed guidsen closed 8 years ago

guidsen commented 8 years ago

Made react router with redux work. Add a base route with a base reducer with some dummy items. Only problem left is the fact that the event handlers aren't being fired. Probably missed something..

guidsen commented 8 years ago

Thanks for the feedback @ericelliott. I've updated the PR. The event handlers still won't fire though. Try to click the button on the screen, the onClick doesn't fire in shared/components/App.js.

ericelliott commented 8 years ago

Have you seen redux-simple-router? See the blog post for details.

guidsen commented 8 years ago

@ericelliott Yeah I saw it. cool, so easy to implement as well. Would you mind checking out what could be wrong with the event handlers not firing like I mentioned earlier?

ericelliott commented 8 years ago

Please fix lint failures. =)

ericelliott commented 8 years ago

This build is failing because you deleted the title component, and there is still a test trying to use it. The correct solution to this problem is to put back the title component (which should not have been deleted. It's an example of how to organize components).

You've also renamed shared/components/app/index.js to App.js -- did you have a good reason to do that?

ericelliott commented 8 years ago

Please note that you could avoid checking in broken builds if you run npm run watch in your terminal and fix any build failures prior to committing your changes. =)

guidsen commented 8 years ago

@ericelliott I renamed shared/components/app/index.js to App.js, because I think it's easier to understand a common used pattern for beginners. I'm not sure where it would make sense to have a sub-folder with a index.js file for a component.

And the reason why I've removed the React parameter for the Title component is because I think it will be easier to understand for new users as well.

ericelliott commented 8 years ago

I renamed shared/components/app/index.js to App.js, because I think it's easier to understand a common used pattern for beginners.

Beginner materials often trivialize things to the point that it's not useful. The priority in this repo is to demonstrate good practices, such as consistently organizing and consistently naming components. =)

And the reason why I've removed the React parameter for the Title component is because I think it will be easier to understand for new users as well.

Same as above. Please keep the factory.

guidsen commented 8 years ago

@ericelliott I've updated it to a factory. It looks and feels better to have a function indeed. What are the benefits of having a factory function to instantiate a component like we do now?

ericelliott commented 8 years ago

What are the benefits of having a factory function to instantiate a component like we do now?

See this article. Having multiple React instances in the same app can cause problems, and users may want to load React from a CDN for the client side. Exporting factories allows them to load React from CDN an inject a single React instance for the app to share.

ericelliott commented 8 years ago

We really should apply this update after the Babel 6 upgrade.

Want to reopen after those changes?

vasco3 commented 8 years ago

I would say that for the next PR to keep it tight to the scope of the issue.. so it's easier/quicker to merge.

ericelliott commented 8 years ago

The idea of this PR is on track. We just need to get it working, and make sure we apply it on top of all the latest modules to make it easier. =)

guidsen commented 8 years ago

Ill try to make the routing work later today. Ill reopen after the Babel 6 upgrade ;).