Skip to content
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.

Sign up for GitHub

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

Jump to bottom

removed options from generator #364

Merged
merged 1 commit into from
Apr 8, 2016
Merged

removed options from generator #364

merged 1 commit into from
Apr 8, 2016

Conversation

jbhatab
Copy link
Member

@jbhatab jbhatab commented Apr 4, 2016

This PR built to remove a few options from the installer and the express server.

Code removed from installer

Heroku
Hot Module Reloader
Express Server
Bootstrap
Ruby and JS linting

If anyone's curious on my logic, I can explain why I removed each in more detail but overall I felt they were outside of the scope of the gem and created maintenance issues.

Documentation and example app

The documentation in the readme hasn't been updated to reflect and that will have to be a dialog on how we want to port that over. Most likely we just need to remove all the relevant documentation and create small 'how tos' for each of these topics, but we can just remove things for now and add the how tos later. In the same vein as this, I also think we need to decide how we are going to move forward with example applications. we have the spec/dummy app and the webpack tutorial. I think we should stick with a single place for these things so we can make maintenance more stable.

How the app is run in develop

I kept the Procfile to run the app and the npm run command in the root package json but I think we should remove both the root package json and the Procfile in favor of manually running both commands. People can setup their own dev enviroments. I think this is outside the scope of this PR though. The relevant messages that pop up when running the generator were changed to reflect this though.

This change is Reviewable

@jbhatab
Copy link
Member Author

jbhatab commented Apr 4, 2016

@justin808 When we finalize this, I'll rebase and it and merge them all into one commit.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.6%) to 84.75% when pulling 76ffd01 on feature/option-removal into 6107015 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.6%) to 84.75% when pulling 76ffd01 on feature/option-removal into 6107015 on master.

@justin808
Copy link
Member

Looking good -- left you some comments.

Reviewed 42 of 42 files at r1.
Review status: all files reviewed at latest revision, 11 unresolved discussions.

CHANGELOG.md, line 9 [r1] (raw file):
5.2.0

CHANGELOG.md, line 10 [r1] (raw file):
removing does not sound like an enhancement

provide some color to the read

README.md, line 104 [r1] (raw file):
or directly on your layout

README.md, line 407 [r1] (raw file):
This is not part of the generator.

We can rebase your changes on top of the doc changes I'm doing. You'll need to manually merge in your README changes.

lib/generators/react_on_rails/templates/base/base/package.json.tt, line 5 [r1] (raw file):
This should be 5.10.0

lib/generators/react_on_rails/templates/base/base/client/package.json.tt, line 89 [r1] (raw file):
Probably don't need this one:

https://github.com/gaearon/babel-plugin-react-transform

lib/generators/react_on_rails/templates/base/base/client/package.json.tt, line 113 [r1] (raw file):
you don't need node-sass and the dev server

lib/generators/react_on_rails/templates/base/base/lib/tasks/assets.rake.tt, line 1 [r1] (raw file):
We refer to this file here: https://github.com/shakacode/react_on_rails/blob/master/docs%2Fadditional-reading%2Fheroku-deployment.md

So we should have this example somewhere before we remove it.

spec/dummy/Gemfile.lock, line 54 [r1] (raw file):
I'd suggest not making changes to /spec/dummy for this PR

spec/dummy/Gemfile.lock, line 260 [r1] (raw file):
why is this changing? this doesn't look right

beta 4 is released

spec/react_on_rails/support/.DS_Store, line 0 [r1] (raw file):
was this file already here? Did you modify this? This type of file should not be in the repo.

Comments from Reviewable

@jbhatab
Copy link
Member Author

jbhatab commented Apr 4, 2016

@justin808 anyway we can disable coveralls on this one since it will definitely reduce coverage from dropped options?

Comments from Reviewable

@jbhatab
Copy link
Member Author

jbhatab commented Apr 4, 2016

README.md, line 407 [r1] (raw file):
@justin808 Is there a branch I can work off of. It would be good to work off of that instead of master.

Comments from Reviewable

@jbhatab
Copy link
Member Author

jbhatab commented Apr 4, 2016

CHANGELOG.md, line 10 [r1] (raw file):
@justin808 how do I add color?

Comments from Reviewable

@justin808
Copy link
Member

Review status: 36 of 42 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

CHANGELOG.md, line 10 [r1] (raw file):
motivations, reasons, etc. Taking away stuff to be easier to support long term (when nothing is broken right now).

Comments from Reviewable

@justin808
Copy link
Member

Getting there. Don't worry about coveralls. Let's make sure the assets.rake file is created by the base installer (not heroku specific), and let's make sure the tests are not removed for this file.

Reviewed 7 of 7 files at r2.
Review status: all files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

docs/additional-reading/heroku-deployment.md, line 68 [r2] (raw file):
Create a rake file to add webpack compilation to asset precompilation. You may already have this file if you used the React on Rails generator.

docs/additional-reading/heroku-deployment.md, line 70 [r2] (raw file):
ruby?

spec/react_on_rails/support/.DS_Store, line [r1] (raw file):
@jbhatab ?

Comments from Reviewable

@jbhatab
Copy link
Member Author

jbhatab commented Apr 5, 2016

CHANGELOG.md, line 10 [r1] (raw file):
I literally meant how do I add color. http://stackoverflow.com/questions/23904274/is-there-a-way-to-get-colored-text-in-github-flavored-markdown. It seems you can't add color anymore without using code blocks which would look weird.

Comments from Reviewable

@jbhatab
Copy link
Member Author

jbhatab commented Apr 5, 2016

spec/react_on_rails/support/.DS_Store, line [r1] (raw file):
I removed it.

Comments from Reviewable

@patrickgordon
Copy link

My thoughts are that this is a positive step - I think it simplifies the gem and separates concerns of what is gem and what can be done with the gem.

I'm happy to help with the documentation around how one might go about adding these things to their project:

  • Heroku
  • Hot Module Reloader
  • Express Server
  • Bootstrap
  • Ruby and JS linting

The suspect that the ones in bold will be the most popular / frequently asked. It might even be worth writing some documents on how to use other CSS frameworks in a general sense. Popular ones like PureCSS or MaterializeCSS come to mind.

Happy to help in any way I can.

@justin808
Copy link
Member

@jbhatab couple small comments. Get CI to pass and I'll merge this.

I also saw you put back in asset.rake, but I couldn't find the test which ensures this file is created.

Reviewed 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

docs/additional-reading/heroku-deployment.md, line 68 [r3] (raw file):

Ensure that you have an `assets.rake` file to ensure that your assets are built for pre-compilation. This file is created by the generator.

lib/generators/react_on_rails/base_generator.rb, line 127 [r3] (raw file):
dots at end of line? blanks?

lib/generators/react_on_rails/templates/base/base/lib/tasks/assets.rake.tt, line 26 [r3] (raw file):
need a new line

Comments from Reviewable

@jbhatab
Copy link
Member Author

jbhatab commented Apr 5, 2016

@justin808 It doesn't show up in the PR anymore because I just reverted it back. It's in there though.

@patrickgordon I think the simplification of this will help a ton so that way other people can start contributing in a more modular fashion like that.

@justin808 I'm thinking we may want to do some more edits on the core readme. Also some of the additional readings stuff may be broken due to the fact they imply you ran the generator. It may be best to just leave them there and mark them in an 'old generator' state while we fix everything up. What do you think?

@jbhatab
Copy link
Member Author

jbhatab commented Apr 6, 2016

lib/generators/react_on_rails/base_generator.rb, line 127 [r3] (raw file):
removed.

Comments from Reviewable

@jbhatab
Copy link
Member Author

jbhatab commented Apr 6, 2016

lib/generators/react_on_rails/templates/base/base/lib/tasks/assets.rake.tt, line 26 [r3] (raw file):
Done.

Comments from Reviewable

@justin808
Copy link
Member

One little comment -- rake is not a language. probably should be ruby.

Reviewed 2 of 3 files at r4.
Review status: all files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

docs/additional-reading/heroku-deployment.md, line 70 [r2] (raw file):
@jbhatab This should be ruby, I'm guessing.

Comments from Reviewable

@justin808
Copy link
Member

Reviewed 1 of 3 files at r4.
Review status: all files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

Comments from Reviewable

justin808
justin808 reviewed Apr 7, 2016
View reviewed changes
README.md
@@ -404,52 +389,6 @@ The best source of docs is the main [ReactOnRails.js](node_package/src/ReactOnRa
setOptions(options)
```

## Hot Reloading View Helpers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbhatab this is not part of the generator change.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.6%) to 84.812% when pulling a35a697 on feature/option-removal into e546bc3 on master.

@justin808
Copy link
Member

:lgtm:

Reviewed 1 of 1 files at r5, 6 of 6 files at r6.
Review status: all files reviewed at latest revision, 13 unresolved discussions.

Comments from Reviewable

@justin808 justin808 merged commit 65d8e9a into master Apr 8, 2016
@justin808 justin808 deleted the feature/option-removal branch April 8, 2016 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jbhatab @coveralls @justin808 @patrickgordon