Merge branch '31552-documentation-dos-and-donts' into 'master'

Resolve "Documentation: Frontend list of do's and don'ts"

Closes #31552

See merge request !11091

doc/development/fe_guide/style_guide_js.md

@@ -11,10 +11,12 @@ See [our current .eslintrc][eslintrc] for specific rules and patterns.
 
 #### ESlint
 
-- **Never** disable eslint rules unless you have a good reason.  You may see a lot of legacy files with `/* eslint-disable some-rule, some-other-rule */` at the top, but legacy files are a special case.  Any time you develop a new feature or refactor an existing one, you should abide by the eslint rules.
-
-- **Never Ever EVER** disable eslint globally for a file
+1. **Never** disable eslint rules unless you have a good reason.  
+You may see a lot of legacy files with `/* eslint-disable some-rule, some-other-rule */`
+at the top, but legacy files are a special case.  Any time you develop a new feature or
+refactor an existing one, you should abide by the eslint rules.
 
+1. **Never Ever EVER** disable eslint globally for a file
   ```javascript
     // bad
     /* eslint-disable */
@@ -26,8 +28,7 @@ See [our current .eslintrc][eslintrc] for specific rules and patterns.
     // nothing :)
   ```
 
-- If you do need to disable a rule for a single violation, try to do it as locally as possible
-
+1. If you do need to disable a rule for a single violation, try to do it as locally as possible
   ```javascript
     // bad
     /* eslint-disable no-new */
@@ -42,9 +43,12 @@ See [our current .eslintrc][eslintrc] for specific rules and patterns.
     // eslint-disable-next-line no-new
     new Foo();
   ```
+1. There are few rules that we need to disable due to technical debt. Which are:
+  1. [no-new][eslint-new]
+  1. [class-methods-use-this][eslint-this]
 
-- When they are needed _always_ place ESlint directive comment blocks on the first line of a script, followed by any global declarations, then a blank newline prior to any imports or code.
-
+1. When they are needed _always_ place ESlint directive comment blocks on the first line of a script,
+followed by any global declarations, then a blank newline prior to any imports or code.
   ```javascript
     // bad
     /* global Foo */
@@ -58,10 +62,9 @@ See [our current .eslintrc][eslintrc] for specific rules and patterns.
     import Bar from './bar';
   ```
 
-- **Never** disable the `no-undef` rule. Declare globals with `/* global Foo */` instead.
-
-- When declaring multiple globals, always use one `/* global [name] */` line per variable.
+1. **Never** disable the `no-undef` rule. Declare globals with `/* global Foo */` instead.
 
+1. When declaring multiple globals, always use one `/* global [name] */` line per variable.
   ```javascript
     // bad
     /* globals Flash, Cookies, jQuery */
@@ -72,8 +75,7 @@ See [our current .eslintrc][eslintrc] for specific rules and patterns.
     /* global jQuery */
   ```
 
-- Use up to 3 parameters for a function or class. If you need more accept an Object instead.
-
+1. Use up to 3 parameters for a function or class. If you need more accept an Object instead.
   ```javascript
     // bad
     fn(p1, p2, p3, p4) {}
@@ -83,8 +85,7 @@ See [our current .eslintrc][eslintrc] for specific rules and patterns.
   ```
 
 #### Modules, Imports, and Exports
-- Use ES module syntax to import modules
-
+1. Use ES module syntax to import modules
   ```javascript
     // bad
     require('foo');
@@ -99,11 +100,10 @@ See [our current .eslintrc][eslintrc] for specific rules and patterns.
     export default Foo;
   ```
 
-- Relative paths
-
-  Unless you are writing a test, always reference other scripts using relative paths instead of `~`
+1. Relative paths: Unless you are writing a test, always reference other scripts using
+relative paths instead of `~`
+  * In **app/assets/javascripts**:
 
-  In **app/assets/javascripts**:
     ```javascript
       // bad
       import Foo from '~/foo'
@@ -111,8 +111,8 @@ See [our current .eslintrc][eslintrc] for specific rules and patterns.
       // good
       import Foo from '../foo';
     ```
+  * In **spec/javascripts**:
 
-  In **spec/javascripts**:
     ```javascript
       // bad
       import Foo from '../../app/assets/javascripts/foo'
@@ -121,10 +121,12 @@ See [our current .eslintrc][eslintrc] for specific rules and patterns.
       import Foo from '~/foo';
     ```
 
-- Avoid using IIFE. Although we have a lot of examples of files which wrap their contents in IIFEs (immediately-invoked function expressions), this is no longer necessary after the transition from Sprockets to webpack. Do not use them anymore and feel free to remove them when refactoring legacy code.
-
-- Avoid adding to the global namespace.
+1. Avoid using IIFE. Although we have a lot of examples of files which wrap their
+contents in IIFEs (immediately-invoked function expressions),
+this is no longer necessary after the transition from Sprockets to webpack.
+Do not use them anymore and feel free to remove them when refactoring legacy code.
 
+1. Avoid adding to the global namespace.
   ```javascript
     // bad
     window.MyClass = class { /* ... */ };
@@ -133,8 +135,7 @@ See [our current .eslintrc][eslintrc] for specific rules and patterns.
     export default class MyClass { /* ... */ }
   ```
 
-- Side effects are forbidden in any script which contains exports
-
+1. Side effects are forbidden in any script which contains exports
   ```javascript
     // bad
     export default class MyClass { /* ... */ }
@@ -146,8 +147,7 @@ See [our current .eslintrc][eslintrc] for specific rules and patterns.
 
 
 #### Data Mutation and Pure functions
-- Strive to write many small pure functions, and minimize where mutations occur.
-
+1. Strive to write many small pure functions, and minimize where mutations occur.
   ```javascript
     // bad
     const values = {foo: 1};
@@ -176,12 +176,11 @@ See [our current .eslintrc][eslintrc] for specific rules and patterns.
     var c = pureFunction(values.foo);
   ```
 
-- Avoid constructors with side-effects
+1. Avoid constructors with side-effects
 
-- Prefer `.map`, `.reduce` or `.filter` over `.forEach`
+1. Prefer `.map`, `.reduce` or `.filter` over `.forEach`
 A forEach will cause side effects, it will be mutating the array being iterated. Prefer using `.map`,
 `.reduce` or `.filter`
-
   ```javascript
     const users = [ { name: 'Foo' }, { name: 'Bar' } ];
 
@@ -197,8 +196,7 @@ A forEach will cause side effects, it will be mutating the array being iterated.
   ```
 
 #### Parse Strings into Numbers
-- `parseInt()` is preferable over `Number()` or `+`
-
+1. `parseInt()` is preferable over `Number()` or `+`
   ```javascript
     // bad
     +'10' // 10
@@ -211,7 +209,7 @@ A forEach will cause side effects, it will be mutating the array being iterated.
   ```
 
 #### CSS classes used for JavaScript
-- If the class is being used in Javascript it needs to be prepend with `js-`
+1. If the class is being used in Javascript it needs to be prepend with `js-`
   ```html
     // bad
     <button class="add-user">
@@ -226,43 +224,61 @@ A forEach will cause side effects, it will be mutating the array being iterated.
 
 ### Vue.js
 
-
 #### Basic Rules
-- Only include one Vue.js component per file.
-- Export components as plain objects:
-
+1. The service has it's own file
+1. The store has it's own file
+1. Use a function in the bundle file to instantiate the Vue component:
   ```javascript
-  export default {
-    template: `<h1>I'm a component</h1>
+    // bad
+    class {
+      init() {
+        new Component({})
+      }
     }
-  ```
 
-#### Naming
-- **Extensions**: Use `.vue` extension for Vue components.
-- **Reference Naming**: Use PascalCase for Vue components and camelCase for their instances:
+    // good
+    document.addEventListener('DOMContentLoaded', () => new Vue({
+      el: '#element',
+      components: {
+        componentName
+      },
+      render: createElement => createElement('component-name'),
+    }));
+  ```
 
+1. Don not use a singleton for the service or the store
   ```javascript
     // bad
-  import cardBoard from 'cardBoard';
+    class Store {
+      constructor() {
+        if (!this.prototype.singleton) {
+          // do something
+        }
+      }
+    }
 
     // good
-  import CardBoard from 'cardBoard'
-
-  // bad
-  components: {
-    CardBoard: CardBoard
-  };
+    class Store {
+      constructor() {
+        // do something
+      }
+    }
+  ```
 
+#### Naming
+1. **Extensions**: Use `.vue` extension for Vue components.
+1. **Reference Naming**: Use camelCase for their instances:
+  ```javascript
     // good
+    import cardBoard from 'cardBoard'
+
     components: {
-    cardBoard: CardBoard
+      cardBoard:
     };
   ```
 
-- **Props Naming:**
-- Avoid using DOM component prop names.
-- Use kebab-case instead of camelCase to provide props in templates.
-
+1. **Props Naming:**  Avoid using DOM component prop names.
+1. **Props Naming:** Use kebab-case instead of camelCase to provide props in templates.
   ```javascript
     // bad
     <component class="btn">
@@ -275,11 +291,10 @@ A forEach will cause side effects, it will be mutating the array being iterated.
 
     // good
     <component my-prop="prop" />
-```
+  ```
 
 #### Alignment
-- Follow these alignment styles for the template method:
-
+1. Follow these alignment styles for the template method:
   ```javascript
     // bad
     <component v-if="bar"
@@ -302,8 +317,7 @@ A forEach will cause side effects, it will be mutating the array being iterated.
   ```
 
 #### Quotes
-- Always use double quotes `"` inside templates and single quotes `'` for all other JS.
-
+1. Always use double quotes `"` inside templates and single quotes `'` for all other JS.
   ```javascript
     // bad
     template: `
@@ -317,8 +331,7 @@ A forEach will cause side effects, it will be mutating the array being iterated.
   ```
 
 #### Props
-- Props should be declared as an object
-
+1. Props should be declared as an object
   ```javascript
     // bad
     props: ['foo']
@@ -333,8 +346,7 @@ A forEach will cause side effects, it will be mutating the array being iterated.
     }
   ```
 
-- Required key should always be provided when declaring a prop
-
+1. Required key should always be provided when declaring a prop
   ```javascript
     // bad
     props: {
@@ -353,8 +365,7 @@ A forEach will cause side effects, it will be mutating the array being iterated.
     }
   ```
 
-- Default key should always be provided if the prop is not required:
-
+1. Default key should always be provided if the prop is not required:
   ```javascript
     // bad
     props: {
@@ -383,7 +394,7 @@ A forEach will cause side effects, it will be mutating the array being iterated.
   ```
 
 #### Data
-- `data` method should always be a function
+1. `data` method should always be a function
 
   ```javascript
     // bad
@@ -401,8 +412,7 @@ A forEach will cause side effects, it will be mutating the array being iterated.
 
 #### Directives
 
-- Shorthand `@` is preferable over `v-on`
-
+1. Shorthand `@` is preferable over `v-on`
   ```javascript
     // bad
     <component v-on:click="eventHandler"/>
@@ -412,8 +422,7 @@ A forEach will cause side effects, it will be mutating the array being iterated.
     <component @click="eventHandler"/>
   ```
 
-- Shorthand `:` is preferable over `v-bind`
-
+1. Shorthand `:` is preferable over `v-bind`
   ```javascript
     // bad
     <component v-bind:class="btn"/>
@@ -424,8 +433,7 @@ A forEach will cause side effects, it will be mutating the array being iterated.
   ```
 
 #### Closing tags
-- Prefer self closing component tags
-
+1. Prefer self closing component tags
   ```javascript
     // bad
     <component></component>
@@ -435,25 +443,51 @@ A forEach will cause side effects, it will be mutating the array being iterated.
   ```
 
 #### Ordering
-- Order for a Vue Component:
+1. Order for a Vue Component:
   1. `name`
-  2. `props`
-  3. `data`
-  4. `components`
-  5. `computedProps`
-  6. `methods`
-  7. lifecycle methods
+  1. `props`
+  1. `mixins`
+  1. `data`
+  1. `components`
+  1. `computedProps`
+  1. `methods`
   1. `beforeCreate`
-    2. `created`
-    3. `beforeMount`
-    4. `mounted`
-    5. `beforeUpdate`
-    6. `updated`
-    7. `activated`
-    8. `deactivated`
-    9. `beforeDestroy`
-    10. `destroyed`
-  8. `template`
+  1. `created`
+  1. `beforeMount`
+  1. `mounted`
+  1. `beforeUpdate`
+  1. `updated`
+  1. `activated`
+  1. `deactivated`
+  1. `beforeDestroy`
+  1. `destroyed`
+
+#### Vue and Boostrap
+1. Tooltips: Do not rely on `has-tooltip` class name for vue components
+  ```javascript
+    // bad
+    <span class="has-tooltip">
+      Text
+    </span>
+
+    // good
+    <span data-toggle="tooltip">
+      Text
+    </span>
+  ```
+
+1. Tooltips: When using a tooltip, include the tooltip mixin
+
+1. Don't change `data-original-title`.
+  ```javascript
+    // bad
+    <span data-original-title="tooltip text">Foo</span>
+
+    // good
+    <span title="tooltip text">Foo</span>
+
+    $('span').tooltip('fixTitle');
+  ```
 
 
 ## SCSS
@@ -461,3 +495,5 @@ A forEach will cause side effects, it will be mutating the array being iterated.
 
 [airbnb-js-style-guide]: https://github.com/airbnb/javascript
 [eslintrc]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/.eslintrc
+[eslint-this]: http://eslint.org/docs/rules/class-methods-use-this
+[eslint-new]: http://eslint.org/docs/rules/no-new

doc/development/fe_guide/vue.md

@@ -387,6 +387,10 @@ describe('Todos App', () => {
   });
 });
 ```
+#### Test the component's output
+The main return value of a Vue component is the rendered output. In order to test the component we
+need to test the rendered output. [Vue][vue-test] guide's to unit test show us exactly that:
+
 
 ### Stubbing API responses
 [Vue Resource Interceptors][vue-resource-interceptor] allow us to add a interceptor with
@@ -419,6 +423,16 @@ the response we need:
   });
 ```
 
+1. Use `$.mount()` to mount the component
+```javascript
+  // bad
+  new Component({
+    el: document.createElement('div')
+  });
+
+  // good
+  new Component().$mount();
+```
 
 [vue-docs]: http://vuejs.org/guide/index.html
 [issue-boards]: https://gitlab.com/gitlab-org/gitlab-ce/tree/master/app/assets/javascripts/boards
@@ -429,5 +443,6 @@ the response we need:
 [one-way-data-flow]: https://vuejs.org/v2/guide/components.html#One-Way-Data-Flow
 [vue-resource-repo]: https://github.com/pagekit/vue-resource
 [vue-resource-interceptor]: https://github.com/pagekit/vue-resource/blob/develop/docs/http.md#interceptors
+[vue-test]: https://vuejs.org/v2/guide/unit-testing.html
 [issue-boards-service]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/assets/javascripts/boards/services/board_service.js.es6
 [flux]: https://facebook.github.io/flux