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.

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

The React On Rails Doctrine #220

Merged
merged 1 commit into from
Jan 26, 2016
Merged

Conversation

justin808
Copy link
Member

First drafts of

  • The React on Rails Doctrine
  • Recommended Project Structure

Review on Reviewable

@justin808 justin808 changed the title ReactOnRails Doct, recommended-project-structure The React On Rails Doctrine Jan 24, 2016
@robwise
Copy link
Contributor

robwise commented Jan 25, 2016

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


docs/coding-style/style.md, line 21 [r1] (raw file):
I think this is confusing because we just linked our own style guide above, we can just let them follow that link and it explains that it's based on AirBnB over at that repo.


docs/doctrine.md, line 3 [r1] (raw file):
I don't think we should start putting authorship on OSS docs because it could discourage participation. Like, I rewrote a lot of the documentation on the README but I didn't put "by Rob Wise" on there or whatever, and I'm providing some editing advice on this. If you do remove the authorship thing, then I would make the rest of this doc use the same narrative as the rest of the documentation.


docs/doctrine.md, line 15 [r1] (raw file):
Sass, actually note the comma.

Also are you sure arbitrarily is the word you want to use here? And this is sort of still the case if we are using bootstrap, and it's not really about positioning so much as it is as namespacing to avoid name collisions


docs/doctrine.md, line 16 [r1] (raw file):
"using Babel and Webpack, is a great language" this could use rewording. How can using something be a language? Also, no comma


docs/doctrine.md, line 17 [r1] (raw file):
You've switched from first-person singular narrative at the begining to first-person plural narrative here (https://en.wikipedia.org/wiki/Narration#Narrative_point_of_view)


docs/doctrine.md, line 20 [r1] (raw file):
setup is really only valid when used as a noun (http://dictionary.reference.com/browse/setup?s=t), I would use set up here


docs/doctrine.md, line 21 [r1] (raw file):
"By placing all client-side development inside of..."

"...can create their own stub.."

The second-to-last sentence is a run-on


docs/doctrine.md, line 25 [r1] (raw file):
now you're also using the 2nd-person narrative, need to keep it consistent


docs/doctrine.md, line 35 [r1] (raw file):
most of this doc you don't put a blank line under a header, which is the way I do it too, but here you do.


docs/doctrine.md, line 42 [r1] (raw file):
I'd say "provide deep links for your client-side application"


docs/doctrine.md, line 43 [r1] (raw file):
it's borderline not a Flux implementation since it does away with some parts of Flux, it's technically a "state container"


docs/doctrine.md, line 48 [r1] (raw file):
The descriptions for each item need to have paralell construction: http://data.grammarbook.com/blog/effective-writing/parallel-construction/


docs/doctrine.md, line 55 [r1] (raw file):
"into" appears twice on either side of "No One Paradigm". I think this sentence could be reworded a little?


docs/doctrine.md, line 58 [r1] (raw file):
this is controversial, but personally I think the 2nd to last item in a list should always have a trailing comma: "with the ShakaCode linters**,**"


docs/doctrine.md, line 61 [r1] (raw file):
"Here's why:"


docs/doctrine.md, line 64 [r1] (raw file):
"allows for trivial set up..."


docs/doctrine.md, line 65 [r1] (raw file):
I'd drop the first two sentences as that's a pretty debatable assertion and is just going to cause nerd rage


docs/doctrine.md, line 74 [r1] (raw file):
"push up" is not an action one would perform on a tent.


docs/doctrine.md, line 75 [r1] (raw file):
The mention of RSpec here seems out of place, especially since Rails uses minitest and not RSpec by default


docs/recommended-project-structure.md, line 3 [r1] (raw file):
"a" not "an", and I would put "enforce" and "recommend" in italics or bold or something to emphasize the difference in wording


docs/recommended-project-structure.md, line 10 [r1] (raw file):
I wouldn't put HelloWorld in lib as an example since that might be confusing and contradicts our generated code


docs/recommended-project-structure.md, line 19 [r1] (raw file):
they're not for JSON files, but for JSON from Ajax requests to be used by the Normalizr package from Dan Abramov.

We are also missing the store folder as is the convention for Redux apps


Comments from the review on Reviewable.io

@justin808
Copy link
Member Author

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


docs/coding-style/style.md, line 21 [r1] (raw file):
removed


docs/doctrine.md, line 3 [r1] (raw file):
we'll just use my name for now


docs/doctrine.md, line 74 [r1] (raw file):
http://rubyonrails.org/doctrine/#big-tent


docs/recommended-project-structure.md, line 19 [r1] (raw file):
@alexfedoseev We're not doing a store folder in our own app. Any take on that?


Comments from the review on Reviewable.io

@justin808
Copy link
Member Author

@robwise 👏 Thanks! See next commit!


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


Comments from the review on Reviewable.io

@alex35mil
Copy link
Member

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


docs/recommended-project-structure.md, line 10 [r1] (raw file):
I'm not sure about this one. I'd use libs as a place for non-bundle-specific commons things, otherwise I see no reason to move bundle-specific code outside the bundle folder.


docs/recommended-project-structure.md, line 19 [r1] (raw file):
We just put it in the startup from the very beginning and never focused on it b/c there was no reason. I have no objections against moving it to store folder 👍


Comments from the review on Reviewable.io

@mapreal19
Copy link
Member

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


Comments from the review on Reviewable.io

@justin808
Copy link
Member Author

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


docs/recommended-project-structure.md, line 19 [r1] (raw file):
Is a store folder worth it if there can only be one store object?


Comments from the review on Reviewable.io

@robwise
Copy link
Contributor

robwise commented Jan 25, 2016

One minor thing left, great work making all of those edits!


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


docs/doctrine.md, line 74 [r1] (raw file):
@justin808 Hey go with it if you want, but it's wrong and DHH is not a native English speaker!


docs/doctrine.md, line 64 [r2] (raw file):
"with support for fragment..."


docs/recommended-project-structure.md, line 19 [r1] (raw file):
@justin808 Your point is perfectly logical and I'm inclined to agree with you, but on the other hand, it's the convention or whatever, and I think it's nice to have the ReactOnRails-specific code in its own folder (startup)


docs/recommended-project-structure.md, line 6 [r2] (raw file):
now that we have widget-editing in the example below, there may some confusion about say "We don't use dashes...for Javascript files"


Comments from the review on Reviewable.io

@justin808
Copy link
Member Author

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


docs/recommended-project-structure.md, line 19 [r1] (raw file):
So store goes in startup or separate store directory? what's the convention @alexfedoseev @robwise @jbhatab ?


Comments from the review on Reviewable.io

@justin808
Copy link
Member Author

couple edits done


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


Comments from the review on Reviewable.io

@robwise
Copy link
Contributor

robwise commented Jan 26, 2016

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


docs/recommended-project-structure.md, line 19 [r1] (raw file):
convention is its own folder: store/configureStore.js: https://github.com/rackt/redux/tree/master/examples/real-world/store


Comments from the review on Reviewable.io

* The React on Rails Doctrine
* Recommended Project Structure
* With lots of review from robwise
@justin808 justin808 force-pushed the the-react-on-rails-doctrine branch from 5a2d739 to 0634d18 Compare January 26, 2016 06:13
justin808 added a commit that referenced this pull request Jan 26, 2016
@justin808 justin808 merged commit 0a5f04e into master Jan 26, 2016
@robwise robwise deleted the the-react-on-rails-doctrine branch January 26, 2016 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants