Skip to content

Conversation

@Judahmeek
Copy link
Collaborator

@Judahmeek Judahmeek commented Aug 19, 2016

An attempt to improve the clarity and focus of the readme. Feel free to cherry-pick through it.


This change is Reviewable

@justin808
Copy link
Member

Looking good. Some small comments.


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


README.md, line 16 [r1] (raw file):

This demo demonstrates support for the some functionality that our React on Rails generators do not currently implement. *We do support this functionality, but we don't generate the code.* Specific functionality in question:

Major enhancements over the basic generated code:


README.md, line 18 [r1] (raw file):

Specific functionality in question: 1. NOTE: Any references to localhost:3000 *might* need to use 0.0.0.0:3000 until Puma fixes an issue regarding this.

This is probably no longer an issue.


README.md, line 19 [r1] (raw file):

1. NOTE: Any references to localhost:3000 *might* need to use 0.0.0.0:3000 until Puma fixes an issue regarding this. 2. **Handling of Sass and Bootstrap**: The tutorial uses CSS modules via Webpack. This is totally different than the older way of having Rails handle Sass/Bootstrap, and having NPM/Webpack handle the Webpack Dev Server. The tutorial now has NPM handle all visual assets. We are using this technique on a new app, and it's awesome!

This comment is pretty old.

There are 2 ways to handle CSS assets:

  1. The Rails asset pipeline.
  2. Using Webpack for CSS Modules.

README.md, line 84 [r1] (raw file):

## Basic Demo Setup 1. Be sure that you have Node installed! We suggest [nvm](https://github.com/creationix/nvm), with node version `v5.0` or above. See this article [Updating and using nvm](http://forum.shakacode.com/t/updating-and-using-nvm/293).

v6 now


README.md, line 123 [r1] (raw file):

**We're now using Webpack for all Sass and JavaScript assets so we can do CSS Modules within Rails!** + **Production Deployment**: We previously had create a file `lib/tasks/assets.rake` to modify the Rails precompile task to deploy assets for production. However, we add this automatically in newer versions of React on Rails. If you need to customize this file, see [lib/tasks/assets.rake from React on Rails](https://github.com/shakacode/react_on_rails/blob/master/lib/tasks/assets.rake) as an example as well as the doc file: [heroku-deployment.md](https://github.com/shakacode/react_on_rails/blob/master/docs/additional-reading/heroku-deployment.md).

previously had created

from beginning of the sentence


README.md, line 216 [r1] (raw file):

[The Shaka Code team!](http://www.shakacode.com/about/), led by [Justin Gordon](https://github.com/justin808/), along with with many others. See [contributors.md](docs/contributors.md) ### About [ShakaCode](http://www.shakacode.com/)

Let's move the About section to the bottom, and as a level 2 header.


README.md, line 220 [r1] (raw file):

If you would like to know more about ShakaCode, please read [Who Is ShakaCode](http://www.shakacode.com/2015/09/17/who-is-shaka-code.html) and [Success the ShakaCode Way!](http://www.shakacode.com/2015/11/26/success-the-shakacode-way.html) Also feel free to visit [our forums!](http://forum.shakacode.com). We've got a [category dedicated to react_on_rails](http://forum.shakacode.com/c/rails/reactonrails).

Please visit ...


Comments from Reviewable

@Judahmeek
Copy link
Collaborator Author

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


README.md, line 16 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Major enhancements over the basic generated code:

I'd prefer not to use the term "Major enhancements" because it signals to junior devs that they NEED this functionality while this entire section is about how such functionality is not provided by default.

It was actually this combination of signals concerning this very functionality that frustrated me enough to submit these pull requests in the first place (well, that and my general frustration with trying to learn React + Redux without learning the entire JS ecosystem)

I think that actually downplaying the significance of this functionality will reduce the concerns of junior devs looking for a gem/tutorial to learn about Reat et al.

Either that, or promoting the fact that we have a tutorial that enables devs to add the functionality that the generators do not provide (which I think is what the hot reloading doc is, but I'm not sure since I haven't yet examined it). Not sure it covers the CSS module functionality, however, and it doesn't look like we have a doc for that yet.


README.md, line 18 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

This is probably no longer an issue.

Will remove.

README.md, line 19 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

This comment is pretty old.

There are 2 ways to handle CSS assets:

  1. The Rails asset pipeline.
  2. Using Webpack for CSS Modules.
This comment may be old, but it still seems valid. However, I'll modify this paragraph to note that react_on_rails still supports (and in fact uses) the Rails asset pipeline.

On a related note, I think that the reason that react_on_rails is designed to support NPM handling all visual assets needs to be promoted more, if only to help junior rails devs overcome the natural resistance to learning new things. It took me a couple of days of seeing node.js come up again and again during my research on client-side frameworks to accept that proper use of client-side frameworks requires embracing node.js and the JS ecosystem.

In fact, while there's a lot of (free and not) programs for teaching people ruby & rails, very few of these programs take the next step of getting their newly minted developers comfortable with client-side frameworks and the JS ecosystem. Might actually be a market opportunity there.


README.md, line 84 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

v6 now

Will update.

README.md, line 123 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

previously had created

from beginning of the sentence

Will correct.

README.md, line 216 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Let's move the About section to the bottom, and as a level 2 header.

Will do!

README.md, line 220 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Please visit ...

gotcha

Comments from Reviewable

@justin808
Copy link
Member

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


README.md, line 16 [r1] (raw file):
How about we replace the first 2 paragraphs:

This tutorial app demonstrates advanced functionality beyond what's provided by the React on Rails generators, mostly in the area of Webpack and React usage. Due to the architecture of placing all client side assets in the /client directory, React on Rails supports just about anything that Webpack and JavaScript can do.


README.md, line 220 [r1] (raw file):

Previously, Judahmeek (Judah Meek) wrote…

gotcha

Please fix.

README.md, line 21 [r2] (raw file):

1. **Hot Reloading with Rails**: The tutorial has different startup scripts than the generators. The dev mode has the WebapackDev server providing the JS and CSS assets to the tutorial. This means you get **HOT RELOADING** of your JS and CSS within your Rails app. If you want to implement CSS Modules and hot reloading after using React on Rails generators, then see [Hot Reloading of Assets For Rails Development](docs/additional-reading/hot-reloading-rails-development.md).

That file does not discuss CSS assets. We need a sibling doc for that.

We could compare and contrast styling using basic Rails with CSS Modules, maybe in a screencast.


Comments from Reviewable

@Judahmeek
Copy link
Collaborator Author

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


README.md, line 16 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

How about we replace the first 2 paragraphs:

This tutorial app demonstrates advanced functionality beyond what's provided by the React on Rails generators, mostly in the area of Webpack and React usage. Due to the architecture of placing all client side assets in the /client directory, React on Rails supports just about anything that Webpack and JavaScript can do.

Nice paragraph.

You want that to replace "Specific functionality in question:" as well as the first paragraph, or do we still want "Specific functionality in question:" to introduce the following list?

Or by "first 2 paragraphs", do you mean that you want to replace the list as well?


README.md, line 21 [r2] (raw file):

Previously, justin808 (Justin Gordon) wrote…

That file does not discuss CSS assets. We need a sibling doc for that.

We could compare and contrast styling using basic Rails with CSS Modules, maybe in a screencast.

I'll change "If you want to implement CSS Modules and hot reloading after..." to "If you want to implement hot reloading after...", add "We are currently working on a tutorial for implementing CSS Modules via Webpack.", and open an issue on react_on_rails regarding the need for a CSS Module tutorial.

Comments from Reviewable

@justin808
Copy link
Member

minor suggestion


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


README.md, line 14 [r3] (raw file):

## NEWS This tutorial app demonstrates advanced functionality beyond what's provided by the React on Rails generators, mostly in the area of Webpack and React usage. Due to the architecture of placing all client side assets in the /client directory, React on Rails supports just about anything that Webpack and JavaScript can do, such as:
`/client` directory 

put ticks around /client


README.md, line 16 [r3] (raw file):

This tutorial uses CSS modules via Webpack so that NPM can manage all visual assets and we gain a better separation of concerns (Rails for server-side functionality & NPM/Webpack for all things client side). However, react_on_rails still supports (and in fact uses) the Rails Asset Pipeline so traditional use of visual assets through the pipeline is possible.

This tutorial uses [CSS modules via Webpack](https://github.com/css-modules/css-modules) so that all your client side configuration can be handled be handled in a pure JavaScript tooling manner. This allows for hot reloading and a better separation of concerns (Rails for server-side functionality & NPM/Webpack for all things client side). The alternative approach of using the traditional Rails Asset Pipeline for your CSS is simpler and supported by React on Rails. --- *[README.md, line 19 \[r3\]](https://reviewable.io:443/reviews/shakacode/react-webpack-rails-tutorial/324#-KPj6ATOVbGFqE1Fv3F5:-KPj6ATOVbGFqE1Fv3F6:bzgdcqa) ([raw file](https://github.com/shakacode/react-webpack-rails-tutorial/blob/4f85597a8455d00d49fb8d2228a05c6971a04263/README.md#L19)):* > If you want to implement hot reloading after using React on Rails generators, then see [Hot Reloading of Assets For Rails Development](docs/additional-reading/hot-reloading-rails-development.md). We are currently working on a tutorial for implementing CSS Modules via Webpack. Move first sentence to end of hot reloading section. Remove second sentence. We'll add it when a tutorial exists. --- *Comments from [Reviewable](https://reviewable.io:443/reviews/shakacode/react-webpack-rails-tutorial/324#-:-KPj87BtrT16JMURjKR_:bdtpgt0)* <!-- Sent from Reviewable.io --> 
@justin808
Copy link
Member

:lgtm_strong:

one tiny suggestion

then rebase -i down to one commit and add on top of master:

git fetch git rebase origin/master 

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


README.md, line 16 [r4] (raw file):

This tutorial app demonstrates advanced functionality beyond what's provided by the React on Rails generators, mostly in the area of Webpack and React usage. Due to the architecture of placing all client side assets in the `/client` directory, React on Rails supports just about anything that Webpack and JavaScript can do, such as: 1. **Handling of Sass and Bootstrap**: This tutorial uses [CSS modules via Webpack](https://github.com/css-modules/css-modules) so that all your client side configuration can be handled be handled in a pure JavaScript tooling manner. This allows for hot reloading and a better separation of concerns (Rails for server-side functionality & NPM/Webpack for all things client side). The alternative approach of using the traditional Rails Asset Pipeline for your CSS is simpler and supported by [React on Rails](https://github.com/shakacode/react_on_rails).

change & to versus


Comments from Reviewable

@justin808
Copy link
Member

Please click the "done" button in reviewable when comments are addressed.


Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


Comments from Reviewable

@justin808
Copy link
Member

2016-08-21_13-28-22

Major changes: + added Table of Contents + removed Notes section + modified intro & news sections + minor grammar & spelling improvements everywhere
@Judahmeek
Copy link
Collaborator Author

Review status: 0 of 1 files reviewed at latest revision, 4 unresolved discussions.


README.md, line 14 [r3] (raw file):

Previously, justin808 (Justin Gordon) wrote…

`/client` directory 

put ticks around /client

Done.

README.md, line 16 [r3] (raw file):

Previously, justin808 (Justin Gordon) wrote…

This tutorial uses CSS modules via Webpack so that NPM can manage all visual assets and we gain a better separation of concerns (Rails for server-side functionality & NPM/Webpack for all things client side). However, react_on_rails still supports (and in fact uses) the Rails Asset Pipeline so traditional use of visual assets through the pipeline is possible.

This tutorial uses [CSS modules via Webpack](https://github.com/css-modules/css-modules) so that all your client side configuration can be handled be handled in a pure JavaScript tooling manner. This allows for hot reloading and a better separation of concerns (Rails for server-side functionality & NPM/Webpack for all things client side). The alternative approach of using the traditional Rails Asset Pipeline for your CSS is simpler and supported by React on Rails. </details> Done. 

README.md, line 19 [r3] (raw file):

Previously, justin808 (Justin Gordon) wrote…

If you want to implement hot reloading after using React on Rails generators, then see Hot Reloading of Assets For Rails Development. We are currently working on a tutorial for implementing CSS Modules via Webpack.

Move first sentence to end of hot reloading section. Remove second sentence. We'll add it when a tutorial exists.

Done.

README.md, line 16 [r4] (raw file):

Previously, justin808 (Justin Gordon) wrote…

change & to versus

Done.

Comments from Reviewable

@justin808
Copy link
Member

:lgtm:


Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


README.md, line 16 [r3] (raw file):

Previously, Judahmeek (Judah Meek) wrote…

Done.

OK

Comments from Reviewable

@justin808
Copy link
Member

Big thanks to @Judahmeek for the help!

@justin808 justin808 merged commit 44a37cd into shakacode:master Aug 22, 2016
@Judahmeek Judahmeek deleted the revise-readme branch August 22, 2016 02:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants