-
-
Notifications
You must be signed in to change notification settings - Fork 627
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
Update CONTRIBUTING.md and related files #737
Conversation
Reviewed 2 of 2 files at r1. CONTRIBUTING.md, line 74 at r1 (raw file):
A test app could be a brand new rails app configured with the React on Rails gem. CONTRIBUTING.md, line 100 at r1 (raw file):
Why is all of this being removed? Unless there is a specific reason for removing one of these steps, please keep them in. OK -- I see, you believe this should go below? CONTRIBUTING.md, line 152 at r1 (raw file):
maybe just make it clear that this is directory to cd to? CONTRIBUTING.md, line 164 at r1 (raw file):
The CONTRIBUTING.md, line 170 at r1 (raw file):
yes, you need to cd here to run these tests CONTRIBUTING.md, line 177 at r1 (raw file):
yes, you need to be here CONTRIBUTING.md, line 220 at r1 (raw file):
I guess we can make it clear what's are the different types of test apps. spec/dummy/client/package.json, line 77 at r1 (raw file):
this looks good if you tried it! and please be sure that when you change the JS part of the npm package, that just running this command works... I'm not 100% sure that this runs the build process on the package. Comments from Reviewable |
Review status: all files reviewed at latest revision, 8 unresolved discussions. CONTRIBUTING.md, line 100 at r1 (raw file): Previously, justin808 (Justin Gordon) wrote…
I'm not entirely comfortable with it being all the way at the bottom too. But it did confuse me by the time I reached the section on Development Setup for Gem and Node Package Contributors. You'd have been introduced to the test app outside the gem, the gem root itself as well as the dummy app. That's three different places you can run Alternatively it can stay in its original place but we must be clear on what is a test app vs a dummy app vs example app and where commands are to be run. CONTRIBUTING.md, line 164 at r1 (raw file): Previously, justin808 (Justin Gordon) wrote…
Thanks for the confirmation. I managed to figure it out eventually. However, it does get confusing if we sometimes call it the dummy app and at other places the test app since we have test apps that live outside the gem as well. Do you think we should be able to distinguish between the two? CONTRIBUTING.md, line 220 at r1 (raw file): Previously, justin808 (Justin Gordon) wrote…
Yes. This is where most of the confusion stems from. spec/dummy/client/package.json, line 77 at r1 (raw file): Previously, justin808 (Justin Gordon) wrote…
Gotcha. I'll confirm the changes. Just to be sure, we want this to be the equivalent of running Comments from Reviewable |
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed. CONTRIBUTING.md, line 100 at r1 (raw file): Previously, wanyamaman (William Wanyama) wrote…
I think up at the top is good as this setup is used for /spec/dummy. Let's make it very clear that the /spec/dummy is one test case for react on rails, and an example of the various techniques! CONTRIBUTING.md, line 164 at r1 (raw file): Previously, wanyamaman (William Wanyama) wrote…
we can just call it the CONTRIBUTING.md, line 170 at r1 (raw file): Previously, justin808 (Justin Gordon) wrote…
Please make it clear you have to be in these directories. spec/dummy/client/package.json, line 77 at r1 (raw file): Previously, wanyamaman (William Wanyama) wrote…
test it out...try it. Comments from Reviewable |
a few comments... Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed. Comments from Reviewable |
@wanyamaman You've got some odd things going on with your git commits. I want to see a PR with ONE commit with your changes. |
e1e0e29
to
80f4f14
Compare
1 similar comment
This is looking FABULOUS. Let's just clarify so that the noob knows that there's 2 parts for the local development reference: 1) ruby part, 2) node part. Reviewed 2 of 2 files at r2. CONTRIBUTING.md, line 82 at r2 (raw file):
nifty graphic! CONTRIBUTING.md, line 85 at r2 (raw file):
and let's be clear on needing to run either the ruby part or the the NPM part depending on if you want to test the ruby or npm parts or BOTH. CONTRIBUTING.md, line 105 at r2 (raw file):
did you try this? CONTRIBUTING.md, line 108 at r2 (raw file):
and not just running yarn install as that might not do anything unless the underlying package.json version number changes. spec/dummy/client/package.json, line 77 at r2 (raw file):
Is the use of the CLIENT_PATH variable required? Nifty if this what it took. we should probably assume that the script anyway if this is required, super.... if not, then let's remove. Comments from Reviewable |
Review status: all files reviewed at latest revision, 13 unresolved discussions, some commit checks failed. CONTRIBUTING.md, line 74 at r1 (raw file): Previously, justin808 (Justin Gordon) wrote…
Done. CONTRIBUTING.md, line 100 at r1 (raw file): Previously, justin808 (Justin Gordon) wrote…
Done. CONTRIBUTING.md, line 152 at r1 (raw file): Previously, justin808 (Justin Gordon) wrote…
Done. CONTRIBUTING.md, line 170 at r1 (raw file): Previously, justin808 (Justin Gordon) wrote…
Done. CONTRIBUTING.md, line 177 at r1 (raw file): Previously, justin808 (Justin Gordon) wrote…
Done. CONTRIBUTING.md, line 85 at r2 (raw file): Previously, justin808 (Justin Gordon) wrote…
I'm not sure what the overall output of this section is meant to be. Please clarify CONTRIBUTING.md, line 93 at r2 (raw file):
This section needs more information. Is this the part when the user wants to test the npm part of the gem. Do we assume they've already installed gem or is this a continuous sequence from the Ruby gem section above? CONTRIBUTING.md, line 105 at r2 (raw file): Previously, justin808 (Justin Gordon) wrote…
Yes, this adds the following line to the CONTRIBUTING.md, line 108 at r2 (raw file): Previously, justin808 (Justin Gordon) wrote…
doesn't the command spec/dummy/client/package.json, line 77 at r2 (raw file): Previously, justin808 (Justin Gordon) wrote…
It depends. If we will have other dummy apps besides the spec dummy then this is a universal script that will work with them all. Otherwise we can hard code the path down from the gem root to the Comments from Reviewable |
80f4f14
to
41311b6
Compare
Reviewed 1 of 1 files at r3. CONTRIBUTING.md, line 85 at r2 (raw file): Previously, wanyamaman (William Wanyama) wrote…
Hi @wanyamaman, the idea is that you will change either the JS part or the Ruby part or both. Depending on which you change, you need to link to the relevant part. One other detail is that the Ruby part just requires restarting the server. The JavaScript part requires re-running the install script and refreshing the browser page. CONTRIBUTING.md, line 93 at r2 (raw file):
CONTRIBUTING.md, line 108 at r2 (raw file): Previously, wanyamaman (William Wanyama) wrote…
Yes, yarn does do spec/dummy/client/package.json, line 77 at r2 (raw file): Previously, wanyamaman (William Wanyama) wrote…
I think we should assume this command is specific to this directory. I'm guessing that the cd to the top level directory is needed to build the node package? I'm surprised we cannot do something like just running the command that you mention above:
(number of directories up might not be right) Comments from Reviewable |
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed. CONTRIBUTING.md, line 164 at r1 (raw file): Previously, justin808 (Justin Gordon) wrote…
Done. CONTRIBUTING.md, line 108 at r2 (raw file): Previously, justin808 (Justin Gordon) wrote…
Hmm I'm not getting the file in client/modules to change. Thoughts? spec/dummy/client/package.json, line 77 at r1 (raw file): Previously, justin808 (Justin Gordon) wrote…
Done. spec/dummy/client/package.json, line 77 at r2 (raw file): Previously, justin808 (Justin Gordon) wrote…
Unfortunately, using yarn add alone isn't triggering the build process for the gem but it does install it in dummy apps dependency list. I'll change the command to be directory specific. The number of directories is fine but lets change it to be more explicit when reading Comments from Reviewable |
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed. CONTRIBUTING.md, line 108 at r2 (raw file): Previously, wanyamaman (William Wanyama) wrote…
this is what needs to be clear. I usually use a command like: https://github.com/shakacode/react_on_rails/blob/master/spec/dummy/package.json#L14
Comments from Reviewable |
41311b6
to
82754ce
Compare
Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions. CONTRIBUTING.md, line 82 at r2 (raw file): Previously, justin808 (Justin Gordon) wrote…
thanks CONTRIBUTING.md, line 85 at r2 (raw file): Previously, justin808 (Justin Gordon) wrote…
I've added clarification. CONTRIBUTING.md, line 108 at r2 (raw file): Previously, justin808 (Justin Gordon) wrote…
Reverted back to npm. Comments from Reviewable |
some comments getting closer! Reviewed 2 of 2 files at r4. CONTRIBUTING.md, line 85 at r4 (raw file):
Testing the Ruby Gem CONTRIBUTING.md, line 94 at r4 (raw file):
Testing the Node package for react-on-rails CONTRIBUTING.md, line 95 at r4 (raw file):
In addition to testing the Ruby parts out, you can test the node package parts of the gem with an external application. CONTRIBUTING.md, line 107 at r4 (raw file):
was this necessary? did you try just running the npm command? CONTRIBUTING.md, line 114 at r4 (raw file):
excellent! but put in issue number!
spec/dummy/client/package.json, line 36 at r4 (raw file):
Is this necessary to put in react_on_rails? what if the project is cloned under a different name? less dots and no react_on_rails maybe! spec/dummy/client/package.json, line 77 at r4 (raw file):
If you really need to delete, then you can add this to the script here. Is this necessary to put in react_on_rails? what if the project is cloned under a different name? less dots and no react_on_rails maybe! Comments from Reviewable |
@wanyamaman Please let me know when you've addressed my comments! |
@wanyamaman Is this ready for merging? |
82754ce
to
50d10b6
Compare
@justin808 updated Review status: 0 of 2 files reviewed at latest revision, 7 unresolved discussions. CONTRIBUTING.md, line 85 at r4 (raw file): Previously, justin808 (Justin Gordon) wrote…
Done. CONTRIBUTING.md, line 94 at r4 (raw file): Previously, justin808 (Justin Gordon) wrote…
Done. CONTRIBUTING.md, line 95 at r4 (raw file): Previously, justin808 (Justin Gordon) wrote…
Done. CONTRIBUTING.md, line 107 at r4 (raw file): Previously, justin808 (Justin Gordon) wrote…
It's not necessary, this was used to clean the CONTRIBUTING.md, line 114 at r4 (raw file): Previously, justin808 (Justin Gordon) wrote…
Done. spec/dummy/client/package.json, line 36 at r4 (raw file): Previously, justin808 (Justin Gordon) wrote…
Done. spec/dummy/client/package.json, line 77 at r4 (raw file): Previously, justin808 (Justin Gordon) wrote…
Done. Comments from Reviewable |
One question...couple minor comments! Close! Reviewed 2 of 2 files at r5. CONTRIBUTING.md, line 113 at r5 (raw file):
file, not function CONTRIBUTING.md, line 115 at r5 (raw file):
Are we sure that the CONTRIBUTING.md, line 116 at r5 (raw file):
that's is only if you have server rendering turned on and you modified the clientStartup file Comments from Reviewable |
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed. CONTRIBUTING.md, line 115 at r5 (raw file): Previously, justin808 (Justin Gordon) wrote…
It does but yarn doesn't. Should we cater for yarn setups or remove that step and the yarn build instruction above until yarn fixes their caching issue? CONTRIBUTING.md, line 116 at r5 (raw file): Previously, justin808 (Justin Gordon) wrote…
Should that be added as a footnote? The assumption here is that you haven't modified spec/dummy so server rendering is on and you've modified the The whole point of this section is to give the reader a general reproducible example highlighting the flow of execution when making a change to the npm package. Unless you'd like to cover all possible configurations they should be able to deduce the changes in their specific configuration. Comments from Reviewable |
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed. CONTRIBUTING.md, line 115 at r5 (raw file): Previously, wanyamaman (William Wanyama) wrote…
I'd mention that, so we know that when yarn fixes that, we'll remove that. CONTRIBUTING.md, line 116 at r5 (raw file): Previously, wanyamaman (William Wanyama) wrote…
I'd keep this all client side, and if the client side works, we'll know the server side works. so your example with Comments from Reviewable |
@wanyamaman Would you like to address my last few comments and we'll merge? |
50d10b6
to
444f048
Compare
Addressed Review status: 1 of 2 files reviewed at latest revision, 3 unresolved discussions. CONTRIBUTING.md, line 113 at r5 (raw file): Previously, justin808 (Justin Gordon) wrote…
updated CONTRIBUTING.md, line 115 at r5 (raw file): Previously, justin808 (Justin Gordon) wrote…
I've removed it and left a footnote on how to do it with yarn once the issue is fixed. CONTRIBUTING.md, line 116 at r5 (raw file): Previously, justin808 (Justin Gordon) wrote…
Moved to client side. Comments from Reviewable |
Reviewed 1 of 1 files at r6. CONTRIBUTING.md, line 113 at r6 (raw file):
I can do this update. CONTRIBUTING.md, line 115 at r6 (raw file):
using Comments from Reviewable |
yarn install-react-on-rails
command usedyarn
instead ofyarn add
and failed to exectute.Closes #736
This change is