From f68f0cd1eacbd9137b014c486c6f6cd6b6378e58 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Thu, 4 May 2017 14:53:00 +0100 Subject: Adds numbered lists to easily point to documentation --- doc/development/fe_guide/style_guide_js.md | 544 ++++++++++++++--------------- doc/development/fe_guide/vue.md | 15 + 2 files changed, 277 insertions(+), 282 deletions(-) diff --git a/doc/development/fe_guide/style_guide_js.md b/doc/development/fe_guide/style_guide_js.md index 1d2b0558948..c8a77f67607 100644 --- a/doc/development/fe_guide/style_guide_js.md +++ b/doc/development/fe_guide/style_guide_js.md @@ -11,207 +11,197 @@ 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 */ + // bad + /* eslint-disable */ - // better - /* eslint-disable some-rule, some-other-rule */ + // better + /* eslint-disable some-rule, some-other-rule */ - // best - // nothing :) + // best + // 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 */ + // bad + /* eslint-disable no-new */ - import Foo from 'foo'; + import Foo from 'foo'; - new Foo(); + new Foo(); - // better - import Foo from 'foo'; + // better + import Foo from 'foo'; - // eslint-disable-next-line no-new - new Foo(); + // 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 */ - /* eslint-disable no-new */ - import Bar from './bar'; + // bad + /* global Foo */ + /* eslint-disable no-new */ + import Bar from './bar'; - // good - /* eslint-disable no-new */ - /* global Foo */ + // good + /* eslint-disable no-new */ + /* global Foo */ - import Bar from './bar'; + 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 */ + // bad + /* globals Flash, Cookies, jQuery */ - // good - /* global Flash */ - /* global Cookies */ - /* global jQuery */ + // good + /* global Flash */ + /* global Cookies */ + /* 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) {} + // bad + fn(p1, p2, p3, p4) {} - // good - fn(options) {} + // good + fn(options) {} ``` #### Modules, Imports, and Exports -- Use ES module syntax to import modules - +1. Use ES module syntax to import modules ```javascript - // bad - require('foo'); + // bad + require('foo'); - // good - import Foo from 'foo'; + // good + import Foo from 'foo'; - // bad - module.exports = Foo; + // bad + module.exports = Foo; - // good - export default Foo; + // good + export default Foo; ``` -- Relative paths +1. Relative paths: Unless you are writing a test, always reference other scripts using relative paths instead of `~` + * In **app/assets/javascripts**: - Unless you are writing a test, always reference other scripts using relative paths instead of `~` + ```javascript + // bad + import Foo from '~/foo' - In **app/assets/javascripts**: - ```javascript - // bad - import Foo from '~/foo' + // good + import Foo from '../foo'; + ``` + * In **spec/javascripts**: - // good - import Foo from '../foo'; - ``` - - In **spec/javascripts**: - ```javascript - // bad - import Foo from '../../app/assets/javascripts/foo' - - // good - import Foo from '~/foo'; - ``` + ```javascript + // bad + import Foo from '../../app/assets/javascripts/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. + // good + import Foo from '~/foo'; + ``` -- 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 { /* ... */ }; + // bad + window.MyClass = class { /* ... */ }; - // good - export default class MyClass { /* ... */ } + // good + 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 { /* ... */ } + // bad + export default class MyClass { /* ... */ } - document.addEventListener("DOMContentLoaded", function(event) { - new MyClass(); - } + document.addEventListener("DOMContentLoaded", function(event) { + new MyClass(); + } ``` #### 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}; + // bad + const values = {foo: 1}; - function impureFunction(items) { - const bar = 1; + function impureFunction(items) { + const bar = 1; - items.foo = items.a * bar + 2; + items.foo = items.a * bar + 2; - return items.a; - } + return items.a; + } - const c = impureFunction(values); + const c = impureFunction(values); - // good - var values = {foo: 1}; + // good + var values = {foo: 1}; - function pureFunction (foo) { - var bar = 1; + function pureFunction (foo) { + var bar = 1; - foo = foo * bar + 2; + foo = foo * bar + 2; - return foo; - } + return foo; + } - var c = pureFunction(values.foo); + 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' } ]; + const users = [ { name: 'Foo' }, { name: 'Bar' } ]; - // bad - users.forEach((user, index) => { - user.id = index; - }); + // bad + users.forEach((user, index) => { + user.id = index; + }); - // good - const usersWithId = users.map((user, index) => { - return Object.assign({}, user, { id: index }); - }); + // good + const usersWithId = users.map((user, index) => { + return Object.assign({}, user, { id: index }); + }); ``` #### Parse Strings into Numbers -- `parseInt()` is preferable over `Number()` or `+` - +1. `parseInt()` is preferable over `Number()` or `+` ```javascript - // bad - +'10' // 10 + // bad + +'10' // 10 - // good - Number('10') // 10 + // good + Number('10') // 10 - // better - parseInt('10', 10); + // better + parseInt('10', 10); ``` #### 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 + - // good - + // good + - + - // if props fit in one line then keep it on the same line - + // if props fit in one line then keep it on the same line + ``` #### 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: ` - - ` - - // good - template: ` - - ` + // bad + template: ` + + ` + + // good + template: ` + + ` ``` #### Props -- Props should be declared as an object - +1. Props should be declared as an object ```javascript - // bad - props: ['foo'] - - // good - props: { - foo: { - type: String, - required: false, - default: 'bar' + // bad + props: ['foo'] + + // good + props: { + foo: { + type: String, + required: false, + default: 'bar' + } } - } ``` -- Required key should always be provided when declaring a prop - +1. Required key should always be provided when declaring a prop ```javascript - // bad - props: { - foo: { - type: String, + // bad + props: { + foo: { + type: String, + } } - } - - // good - props: { - foo: { - type: String, - required: false, - default: 'bar' + + // good + props: { + foo: { + type: String, + required: false, + default: 'bar' + } } - } ``` -- 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: { - foo: { - type: String, - required: false, + // bad + props: { + foo: { + type: String, + required: false, + } } - } - - // good - props: { - foo: { - type: String, - required: false, - default: 'bar' + + // good + props: { + foo: { + type: String, + required: false, + default: 'bar' + } } - } - // good - props: { - foo: { - type: String, - required: true + // good + props: { + foo: { + type: String, + required: true + } } - } ``` #### Data -- `data` method should always be a function +1. `data` method should always be a function ```javascript - // bad - data: { - foo: 'foo' - } - - // good - data() { - return { + // bad + data: { foo: 'foo' - }; - } + } + + // good + data() { + return { + foo: 'foo' + }; + } ``` #### Directives -- Shorthand `@` is preferable over `v-on` - +1. Shorthand `@` is preferable over `v-on` ```javascript - // bad - + // bad + - // good - + // good + ``` -- Shorthand `:` is preferable over `v-bind` - +1. Shorthand `:` is preferable over `v-bind` ```javascript - // bad - + // bad + - // good - + // good + ``` #### Closing tags -- Prefer self closing component tags - +1. Prefer self closing component tags ```javascript - // bad - + // bad + - // good - + // good + ``` #### 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. `beforeCreate` - 2. `created` - 3. `beforeMount` - 4. `mounted` - 5. `beforeUpdate` - 6. `updated` - 7. `activated` - 8. `deactivated` - 9. `beforeDestroy` - 10. `destroyed` - 8. `template` + 1. `props` + 1. `mixins` + 1. `data` + 1. `components` + 1. `computedProps` + 1. `methods` + 1. `beforeCreate` + 1. `created` + 1. `beforeMount` + 1. `mounted` + 1. `beforeUpdate` + 1. `updated` + 1. `activated` + 1. `deactivated` + 1. `beforeDestroy` + 1. `destroyed` ## SCSS @@ -461,3 +439,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 diff --git a/doc/development/fe_guide/vue.md b/doc/development/fe_guide/vue.md index 73d2ffc1bdc..a984bb6c94c 100644 --- a/doc/development/fe_guide/vue.md +++ b/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 -- cgit v1.2.1