-
Couldn't load subscription status.
- Fork 380
Client refactoring #96
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
Conversation
6ab98db to
1ec74a0
Compare
@alexfedoseev Yes, let's move /client/assets/javascripts to /client/app and update the README.md. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be changed to app-bundle.js and vendor-bundle.js?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dylangrafmyre Nice catch! Will fix it today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexfedoseev @dylangrafmyre Should we also upgrade to current npm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and to current node also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also when we upgrade to npm@3.3.4 we need to change the npm install/upgrade instructions in the client/REAMDE. shrinkwrap.json gets automatically updated during npm install/upgrade similar to Gemfile.loc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By saying "yes", I meant current npm@2.x.x. I suggest to wait a month or so before upgrade to 3.x.x, since there is performance issue.
.gitignore
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@alexfedoseev Let's try this out with the master on react_on_rails, and if that works, I'm going to push the next version of react_on_rails.
I just pushed up the new version of https://rubygems.org/gems/react_on_rails. Let's get that in here, and let's put in server rendering.
fe4aeaa to
b06322e
Compare
b06322e to
06fc11e
Compare
@justin808 Ready to review!
P.S. I haven't applied component-oriented structure and normalizr in this PR, will do it in the next one.
@alexfedoseev Please update to the latest react_on_rails gem.
@samnang @dylangrafmyre Any feedback on this one?
LGTM
@justin808 I've updated gem to 0.1.5, but all html from server is escaped now: http://share.minima.pro/dMgm
@alexfedoseev if all looks good, please merge and push latest to Heroku 👍
@justin808 FYI, Codeship automatically deploys to heroku any merges to master.
@justin808 @dylangrafmyre Merged!
npm runscripts/client/app@justin808 Should we make
/client/assets/javascripts->/client/apptransition here?