Style Guide updates

developer/code-reviews.md

@@ -0,0 +1,46 @@
+# Code review process
+
+Code contributions are reviewed by the Reaction core and community contributors. All pull requests are reviewed and approved by at least one core contributor before merge.
+
+Here's what to expect when you make a pull request to Reaction:
+
+## Automated pull request checks
+
+As soon as pull requests are pushed, [BitHound](https://www.bithound.io/github/reactioncommerce/reaction), [CircleCI](https://circleci.com/gh/reactioncommerce/reaction) and [Synk](https://snyk.io/) run automated tests. All checks should pass successfully to ensure:
+
+- All linting rules pass withour errors or ignores.
+- Dependencies are not failing.
+- All tests pass.
+- No new security vulnerabilities are introduced.
+
+You can run these checks yourself by running these commands:
+- `eslint .` for linting
+- `reaction test` for all tests
+
+## Core team pull request review
+
+Every Monday, the Core team triages all new pull requests. The Core team reviews code quality rules including:
+
+- No new Atmosphere or Meteor dependencies are introduced.
+- No hard-coded copy: All copy and alerts should have i18n keys and values.
+- Updated LingoHub translations.
+- All new methods and files have jsdoc summaries, as outlined in [JSDoc guide](https://github.com/reactioncommerce/reaction-jsdoc#how-to-write-docs).
+- All folders, variables, method names follow naming conventions, outlined in [Reaction style guide](/developer/styleguide.md).
+
+The Core team also encourages in-line commenting. Use comments to:
+- Explain functionality to someone new to the code
+- Link to any external documentation
+
+
+## Getting feedback early and often
+
+Want to get feedback on your pull request before it's ready for merging?
+
+Push up the branch and add `[WIP]` for Work in Progress to the title and ask a question in GitHub.
+
+## Congrats! It's approved and merged. What's next?
+
+Once a pull request goes through both the automated and Core team reviews, it's ready to be merged. Here are some things you may want to consider after that:
+
+- If your pull request referenced an issue, close that issue.
+- Does your new feature require new user documentation or developer documentation? Make an issue for that in [reaction-docs](https://github.com/reactioncommerce/reaction-docs/issues).

developer/styleguide.md

@@ -1,119 +1,216 @@
-# Style Guide
+# Style guide
 
-Our rules are similar to AirBnB or Meteor rules, [standard template of ESLint rules](https://www.npmjs.com/package/eslint-config-airbnb), although we have tweaked them a little with what is working best for us.
+As a community, Reaction follows guidelines for JavaScript style and syntax conventions, along with guidelines for naming variables, methods and filenames.
 
-A couple of notable Reaction specific style rules:
+## Syntax and style conventions
 
-- Double quoted strings
-- Well spaced function
-- 160 character line length
+Our rules are similar to [Airbnb JavaScript Style Guide](https://github.com/airbnb/javascript) and [Meteor Code Style](https://guide.meteor.com/code-style.html), [standard template of ESLint rules](https://www.npmjs.com/package/eslint-config-airbnb), with a few custom Reaction-specific rules:
+
+- Double-quoted strings
+- Well-spaced functions
+- Spaces around brackets
+- 120 character line-length
 - `import` order
   - React npm packages (`React`, `prop-types`, etc...)
   - other npm packages
-  - meteor core packages
-  - meteor (Atmosphere) packages
+  - Meteor core packages
+  - Meteor (Atmosphere) packages
   - local app files
 
-Review [Meteor Code Style](https://guide.meteor.com/code-style.html) for additional guidelines that are typical of Meteor projects.
+Reaction uses various linting libraries to automate style checking. Find the exact rules in the code:
+- [`.eslintrc`](https://github.com/reactioncommerce/reaction/blob/master/.eslintrc) - [ESLint](http://eslint.org) checks JavaScript style, including [ES2015](https://docs.meteor.com/packages/ecmascript.html#Supported-ES2015-Features), React and Babel.
+- [`.jsbeautifyrc`](https://github.com/reactioncommerce/reaction/blob/master/.jsbeautifyrc) - [JS Beautifier](jsbeautifier.org) automates code formatting
+- [`.editorconfig`](https://github.com/reactioncommerce/reaction/blob/master/.editorconfig) - [Editor Config](https://editorconfig.org/) standardizes file formatting
+
+To see the rules in action, run `eslint .` from the command line or use [ESLint code editor tools](https://eslint.org/docs/user-guide/integrations).
 
-## Editor Configuration
+💡 **ProTip!** If you're using [Atom](https://atom.io/), like the Core team, you can install all the necessary tools in one line:  `apm install editorconfig atom-beautify linter linter-eslint linter-markdown linter-jsonlint linter-docker`
 
-In the Reaction app root, we have Reaction specific configuration files that can be used with most editors with the appropriate tools installed.
+## Naming conventions
 
-- `.eslintrc` - [http://eslint.org/](https://eslint.org/)
-- `.jsbeautifyrc` - [http://jsbeautifier.org/](https://jsbeautifier.org/)
-- `.editorconfig` - [http://editorconfig.org/](https://editorconfig.org/)
+### Avoid camel-casing files and folders
 
-These configurations also include additional rules supporting [ES2015](https://docs.meteor.com/packages/ecmascript.html#Supported-ES2015-Features) and `React`.
+Not all operating systems are case sensitive, so avoid having two files named the same with differing case. Instead of camel-casing, use hyphens. Underscores are not to be used for file or folder names unless expressly required.
 
-### Atom
+Names of folders should be:
+- Lowercased
+- Alphanumeric characters
+- Hyphenated to join more words
+- Avoid camelCase, undescore_casing
 
-We've been using the Open Source [Atom](https://atom.io/) editor for some time now and can recommend it.
+File names follow the same conventions as folder names, and also allow for names to contain multiple (.) characters as needed.
 
-Setting up Atom is a quick command where we use the shell command `apm` to install the same [Atom linting packages](https://atom.io/users/AtomLinter) as we use for Reaction.
+**Do**
 
 ```sh
-apm install editorconfig atom-beautify linter linter-eslint linter-markdown linter-jsonlint linter-docker
+  ui-grid/
+  example-payment-method/
+  social/
 ```
 
-### Lint
+```sh
+  bootstrap.rtl.js
+  index.js
+```
 
-Reaction installs the `eslint` v3.x packages *eslint*, *eslint-plugin-react*, *babel-eslint* from [npm](https://www.npmjs.com/) as development dependencies.
+**Don't**
 
-In the Reaction app root, we have a Reaction `eslint` configuration.
+```sh
+  reactionpackagename/
+  address_book/
+  addressBook/
+```
 
-`.eslintrc` - [http://eslint.org/](https://eslint.org/)
+```sh
+  settingsContainer.js
+  addressBook.js
+  address_book.js
+```
 
-For more details, see the [ESLint Getting Started Guide](http://eslint.org/docs/user-guide/getting-started).
+### Create strong package names
 
-#### Markdown
+Name package folders in this format: `<functionality>-<package-name>`
 
-In our Markdown documentation, we use [remark-lint](https://github.com/wooorm/remark-lint) to ensure consistent Markdown styling.
+```sh
+/imports/plugins/custom/payments-custom-provider
+/imports/plugins/included/shipping-provider
+```
 
-### Pull Requests
+Registry package names should preferably use some organizational namespacing as a prefix.
 
-Pull request branches are evaluated using [BitHound](https://www.bithound.io/github/reactioncommerce/reaction) for insecure dependencies and code quality.
+```js
+reaction-paypal
+reaction-google-analytics
+yourorg-your-package
+```
 
-- must pass a Linting check and *no errors* are accepted.
-- must pass a Dependency check and no priority packages are accepted.
-- must pass `reaction test`
+### Group related files together into folders
 
-In many cases, documentation updates can be required as well.
+As a convention, if there are multiple files that provide functionality that is broken down across multiple files, these files should be grouped within a folder. If the files are stand-alone and the name needs to represent functionality, you can use a hyphen for the separator.
 
-Pull requests are submitted to a peer code review process before acceptance.
+**Do**
 
-### File Naming Conventions
+```sh
+/container/settings.js
+/container/settings.html
+/container/ <more files>
+```
+
+or
 
-In general we use hyphens (-) and camelCase for folder names, and camelCase alone for file names. Underscores are not to be used for file or folder names unless expressly required.  Be aware that not all operating systems are case sensitive, so it's not ok to have two files named the same with differing case.
+```sh
+/settings-container.js
+/settings-container.html
+```
 
-#### Folder Names
+**Don't**
 
-**Good**
+```sh
+/container/settings-containers.js
+/container/settingsContainer.html
+settingsContainer.js
+```
 
-- Folder names for packages must contain only lowercased alpha numeric characters and may be hyphenated if joining more than one word
-- Folder names all normal directories must start with a lowercased letter and may camel cased if joining more than one word
+## JavaScript style recommendations
 
+## Variables
 
-    // Packages in /imports/plugins
-    ui-grid/
-    example-paymentmethod/
-    social/
-    taxes-avalara/
+### Naming variables
 
-    // All other folder names everywhere
-    addressBook/
+Variable names should be:
 
-**Bad**
+- Meaningful. Avoid single-letter variables.
+- Use `error`, instead of `e` or `err`
+- Variables that contains a boolean value should use a `isThisTrue` style.
+- Variables that return arrays should be plural.
 
-- Package name should contain hyphens to make it easier to read.
-- Underscores are not to be used unless expressly required.
+Publication names should:
 
+- Use TitleCase
 
-    reactionpackagename/
-    address_book/
+### Working with variables
 
-#### File Names
+- Use `const` except when a variable is being reassigned.
 
-**Good**
+## Methods
 
-- File names must start with a lowercased letter and may be camel cased if joining more than one word.
-- File names may contain multiple (.) characters as needed
+### Method names should:
+- Methods names should use a verb/noun style when possible, e.g. `checkConfiguration` or `addToCart`.
+- Methods that return a single value should be singular.
+- Methods whose main purpose is to return a value should use the get prefix.
 
+- Avoid ternary operators and one-line `if` statements
 
-    settingsContainer.js
-    publishContainer.js
-    addressBook.js
-    bootstrap.rtl.js
-    index.js
+### Use parenthesis for clarity
 
-    // This is an exception as it's part of Meteor's naming convention
-    addressBook.app-test.js
+- Use parenthesis around all method arguments.
 
-**Bad**
+**Don't**
+```
+cartItems.find(item => item.status === picked)
+```
+
+**Do**
+```
+cartItems.find((item) => item.status === picked)
+```
+
+- When specifying a callback, always use the parenthesis to indicate the argument being accepted over the bare arrow-function form. This way, it's clear it's not another argument to the original function:
+
+**Don't**
+```
+thisMethodTakesACallback(arg1, arg2, result => {
+  doStuff
+});
+```
+**Do**
+
+```
+thisMethodTakesACallback(arg1, arg2, (result) => {
+  dostuff();
+});
+```
+
+
+### Avoid multi-line shorthand arrow functions
+
+If you have to break your arrow function into multiple lines, use curly brackets and a return rather than trying to break the shorthand arrow function into multiple lines.
+
+
+**Don't**
+ ```
+cartItems.find(
+   (item) => item.status === status
+ )
+```
+
+**Do**
+ ```
+cartItems.find((item) => {
+  return item.status === status;
+})
+```
+
+### Working with collections
+
+Be explicit in querying:
+
+**Don't**
+```
+Products.findOne("abc123")
+```
+**Do**
+```
+Products.findOne({ _id: "abc123" })
+```
 
-- Hyphens and underscores are not to be used unless expressly required; such is the case with Meteor for `*.app-test.js` files. Or for migration files to make the filename more readable.
+### Replacing lodash with ES2015
 
+Use native ES6 elements over lodash whenever there is a 1-for-1 replacement. Here are some examples of lodash methods that can be replaced with native elements:
 
-    settings_container.js
-    publish-container.js
-    address_book.js
+- Replace `_.map`, with  `Array.prototype.map`
+- Replace `_.reduce`, with  `Array.prototype.reduce`
+- Replace `_.filter`, with  `Array.prototype.filter`
+- Replace `_.each`, `_.forEach`, with  `Array.prototype.forEach`
+- Replace `_.isArray`, with `Array.isArray`
+- Replace `_.extend` with `Object.assign`