diff options
Diffstat (limited to 'doc/development/new_fe_guide/style')
-rw-r--r-- | doc/development/new_fe_guide/style/html.md | 28 | ||||
-rw-r--r-- | doc/development/new_fe_guide/style/javascript.md | 312 |
2 files changed, 162 insertions, 178 deletions
diff --git a/doc/development/new_fe_guide/style/html.md b/doc/development/new_fe_guide/style/html.md index 035fcbb28df..e8c9c2ccebf 100644 --- a/doc/development/new_fe_guide/style/html.md +++ b/doc/development/new_fe_guide/style/html.md @@ -2,11 +2,11 @@ ## Buttons -<a name="button-type"></a><a name="1.1"></a> +### Button type -- [1.1](#button-type) **Use button type** Button tags requires a `type` attribute according to the [W3C HTML specification][button-type-spec]. +Button tags requires a `type` attribute according to the [W3C HTML specification](https://www.w3.org/TR/2011/WD-html5-20110525/the-button-element.html#dom-button-type). -``` +```html // bad <button></button> @@ -14,11 +14,11 @@ <button type="button"></button> ``` -<a name="button-role"></a><a name="1.2"></a> +### Button role -- [1.2](#button-role) **Use button role for non buttons** If an HTML element has an onClick handler but is not a button, it should have `role="button"`. This is more [accessible][button-role-accessible]. +If an HTML element has an `onClick` handler but is not a button, it should have `role="button"`. This is [more accessible](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_button_role). -``` +```html // bad <div onClick="doSomething"></div> @@ -28,11 +28,11 @@ ## Links -<a name="blank-links"></a><a name="2.1"></a> +### Blank target -- [2.1](#blank-links) **Use rel for target blank** Use `rel="noopener noreferrer"` whenever your links open in a new window i.e. `target="_blank"`. This prevents [the following][jitbit-target-blank] security vulnerability documented by JitBit +Use `rel="noopener noreferrer"` whenever your links open in a new window, i.e. `target="_blank"`. This prevents a security vulnerability [documented by JitBit](https://www.jitbit.com/alexblog/256-targetblank---the-most-underestimated-vulnerability-ever/). -``` +```html // bad <a href="url" target="_blank"></a> @@ -40,18 +40,14 @@ <a href="url" target="_blank" rel="noopener noreferrer"></a> ``` -<a name="fake-links"></a><a name="2.2"></a> +### Fake links -- [2.2](#fake-links) **Do not use fake links** Use a button tag if a link only invokes JavaScript click event handlers. This is more semantic. +**Do not use fake links.** Use a button tag if a link only invokes JavaScript click event handlers, which is more semantic. -``` +```html // bad <a class="js-do-something" href="#"></a> // good <button class="js-do-something" type="button"></button> ``` - -[button-type-spec]: https://www.w3.org/TR/2011/WD-html5-20110525/the-button-element.html#dom-button-type -[button-role-accessible]: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_button_role -[jitbit-target-blank]: https://www.jitbit.com/alexblog/256-targetblank---the-most-underestimated-vulnerability-ever/ diff --git a/doc/development/new_fe_guide/style/javascript.md b/doc/development/new_fe_guide/style/javascript.md index 7985b893c9e..3019eaa089c 100644 --- a/doc/development/new_fe_guide/style/javascript.md +++ b/doc/development/new_fe_guide/style/javascript.md @@ -1,208 +1,196 @@ # JavaScript style guide -We use [Airbnb's JavaScript Style Guide][airbnb-style-guide] and it's accompanying linter to manage most of our JavaScript style guidelines. +We use [Airbnb's JavaScript Style Guide](https://github.com/airbnb/javascript) and it's accompanying +linter to manage most of our JavaScript style guidelines. -In addition to the style guidelines set by Airbnb, we also have a few specific rules listed below. +In addition to the style guidelines set by Airbnb, we also have a few specific rules +listed below. > **Tip:** You can run eslint locally by running `yarn eslint` -## Arrays +## Avoid forEach + +Avoid forEach when mutating data. Use `map`, `reduce` or `filter` instead of `forEach` +when mutating data. This will minimize mutations in functions, +which aligns with [Airbnb's style guide](https://github.com/airbnb/javascript#testing--for-real). + +```javascript +// bad +users.forEach((user, index) => { + user.id = index; +}); + +// good +const usersWithId = users.map((user, index) => { + return Object.assign({}, user, { id: index }); +}); +``` + +## Limit number of parameters + +If your function or method has more than 3 parameters, use an object as a parameter +instead. + +```javascript +// bad +function a(p1, p2, p3) { + // ... +}; + +// good +function a(p) { + // ... +}; +``` + +## Avoid side effects in constructors + +Avoid making asynchronous calls, API requests or DOM manipulations in the `constructor`. +Move them into separate functions instead. This will make tests easier to write and +code easier to maintain. -<a name="avoid-foreach"></a><a name="1.1"></a> +```javascript +// bad +class myClass { + constructor(config) { + this.config = config; + axios.get(this.config.endpoint) + } +} -- [1.1](#avoid-foreach) **Avoid ForEach when mutating data** Use `map`, `reduce` or `filter` instead of `forEach` when mutating data. This will minimize mutations in functions ([which is aligned with Airbnb's style guide][airbnb-minimize-mutations]) +// good +class myClass { + constructor(config) { + this.config = config; + } - ``` - // bad - users.forEach((user, index) => { - user.id = index; - }); + makeRequest() { + axios.get(this.config.endpoint) + } +} +const instance = new myClass(); +instance.makeRequest(); - // good - const usersWithId = users.map((user, index) => { - return Object.assign({}, user, { id: index }); - }); - ``` +``` -## Functions +## Avoid classes to handle DOM events -<a name="limit-params"></a><a name="2.1"></a> +If the only purpose of the class is to bind a DOM event and handle the callback, prefer +using a function. -- [2.1](#limit-params) **Limit number of parameters** If your function or method has more than 3 parameters, use an object as a parameter instead. +```javascript +// bad +class myClass { + constructor(config) { + this.config = config; + } - ``` - // bad - function a(p1, p2, p3) { - // ... - }; + init() { + document.addEventListener('click', () => {}); + } +} - // good - function a(p) { - // ... - }; - ``` +// good -## Classes & constructors +const myFunction = () => { + document.addEventListener('click', () => { + // handle callback here + }); +} +``` -<a name="avoid-constructor-side-effects"></a><a name="3.1"></a> +## Pass element container to constructor -- [3.1](#avoid-constructor-side-effects) **Avoid side effects in constructors** Avoid making some operations in the `constructor`, such as asynchronous calls, API requests and DOM manipulations. Prefer moving them into separate functions. This will make tests easier to write and code easier to maintain. +When your class manipulates the DOM, receive the element container as a parameter. +This is more maintainable and performant. - ```javascript - // bad - class myClass { - constructor(config) { - this.config = config; - axios.get(this.config.endpoint) - } - } +```javascript +// bad +class a { + constructor() { + document.querySelector('.b'); + } +} - // good - class myClass { - constructor(config) { - this.config = config; - } +// good +class a { + constructor(options) { + options.container.querySelector('.b'); + } +} +``` - makeRequest() { - axios.get(this.config.endpoint) - } - } - const instance = new myClass(); - instance.makeRequest(); +## Use ParseInt - ``` +Use `ParseInt` when converting a numeric string into a number. -<a name="avoid-classes-to-handle-dom-events"></a><a name="3.2"></a> +```javascript +// bad +Number('10') -- [3.2](#avoid-classes-to-handle-dom-events) **Avoid classes to handle DOM events** If the only purpose of the class is to bind a DOM event and handle the callback, prefer using a function. +// good +parseInt('10', 10); +``` - ``` - // bad - class myClass { - constructor(config) { - this.config = config; - } +## CSS Selectors - Use `js-` prefix - init() { - document.addEventListener('click', () => {}); - } - } +If a CSS class is only being used in JavaScript as a reference to the element, prefix +the class name with `js-`. - // good +```html +// bad +<button class="add-user"></button> - const myFunction = () => { - document.addEventListener('click', () => { - // handle callback here - }); - } - ``` +// good +<button class="js-add-user"></button> +``` -<a name="element-container"></a><a name="3.3"></a> +## Absolute vs relative paths for modules -- [3.3](#element-container) **Pass element container to constructor** When your class manipulates the DOM, receive the element container as a parameter. - This is more maintainable and performant. +Use relative paths if the module you are importing is less than two levels up. - ``` - // bad - class a { - constructor() { - document.querySelector('.b'); - } - } +```javascript +// bad +import GitLabStyleGuide from '~/guides/GitLabStyleGuide'; - // good - class a { - constructor(options) { - options.container.querySelector('.b'); - } - } - ``` +// good +import GitLabStyleGuide from '../GitLabStyleGuide'; +``` -## Type Casting & Coercion +If the module you are importing is two or more levels up, use an absolute path instead: -<a name="use-parseint"></a><a name="4.1"></a> +```javascript +// bad +import GitLabStyleGuide from '../../../guides/GitLabStyleGuide'; -- [4.1](#use-parseint) **Use ParseInt** Use `ParseInt` when converting a numeric string into a number. +// good +import GitLabStyleGuide from '~/GitLabStyleGuide'; +``` - ``` - // bad - Number('10') +Additionally, **do not add to global namespace**. - // good - parseInt('10', 10); - ``` +## Do not use `DOMContentLoaded` in non-page modules -## CSS Selectors +Imported modules should act the same each time they are loaded. `DOMContentLoaded` +events are only allowed on modules loaded in the `/pages/*` directory because those +are loaded dynamically with webpack. -<a name="use-js-prefix"></a><a name="5.1"></a> +## Avoid XSS -- [5.1](#use-js-prefix) **Use js prefix** If a CSS class is only being used in JavaScript as a reference to the element, prefix the class name with `js-` +Do not use `innerHTML`, `append()` or `html()` to set content. It opens up too many +vulnerabilities. - ``` - // bad - <button class="add-user"></button> +## Disabling ESLint in new files - // good - <button class="js-add-user"></button> - ``` +Do not disable ESLint when creating new files. Existing files may have existing rules +disabled due to legacy compatibility reasons but they are in the process of being refactored. -## Modules +Do not disable specific ESLint rules. Due to technical debt, you may disable the following +rules only if you are invoking/instantiating existing code modules. -<a name="use-absolute-paths"></a><a name="6.1"></a> + - [no-new](http://eslint.org/docs/rules/no-new) + - [class-method-use-this](http://eslint.org/docs/rules/class-methods-use-this) -- [6.1](#use-absolute-paths) **Use absolute paths for nearby modules** Use absolute paths if the module you are importing is less than two levels up. - - ``` - // bad - import GitLabStyleGuide from '~/guides/GitLabStyleGuide'; - - // good - import GitLabStyleGuide from '../GitLabStyleGuide'; - ``` - -<a name="use-relative-paths"></a><a name="6.2"></a> - -- [6.2](#use-relative-paths) **Use relative paths for distant modules** If the module you are importing is two or more levels up, use a relative path instead of an absolute path. - - ``` - // bad - import GitLabStyleGuide from '../../../guides/GitLabStyleGuide'; - - // good - import GitLabStyleGuide from '~/GitLabStyleGuide'; - ``` - -<a name="global-namespace"></a><a name="6.3"></a> - -- [6.3](#global-namespace) **Do not add to global namespace** - -<a name="domcontentloaded"></a><a name="6.4"></a> - -- [6.4](#domcontentloaded) **Do not use DOMContentLoaded in non-page modules** Imported modules should act the same each time they are loaded. `DOMContentLoaded` events are only allowed on modules loaded in the `/pages/*` directory because those are loaded dynamically with webpack. - -## Security - -<a name="avoid-xss"></a><a name="7.1"></a> - -- [7.1](#avoid-xss) **Avoid XSS** Do not use `innerHTML`, `append()` or `html()` to set content. It opens up too many vulnerabilities. - -## ESLint - -<a name="disable-eslint-file"></a><a name="8.1"></a> - -- [8.1](#disable-eslint-file) **Disabling ESLint in new files** Do not disable ESLint when creating new files. Existing files may have existing rules disabled due to legacy compatibility reasons but they are in the process of being refactored. - -<a name="disable-eslint-rule"></a><a name="8.2"></a> - -- [8.2](#disable-eslint-rule) **Disabling ESLint rule** Do not disable specific ESLint rules. Due to technical debt, you may disable the following rules only if you are invoking/instantiating existing code modules - - - [no-new][no-new] - - [class-method-use-this][class-method-use-this] - -> Note: Disable these rules on a per line basis. This makes it easier to refactor in the future. E.g. use `eslint-disable-next-line` or `eslint-disable-line` - -[airbnb-style-guide]: https://github.com/airbnb/javascript -[airbnb-minimize-mutations]: https://github.com/airbnb/javascript#testing--for-real -[no-new]: http://eslint.org/docs/rules/no-new -[class-method-use-this]: http://eslint.org/docs/rules/class-methods-use-this +> Note: Disable these rules on a per line basis. This makes it easier to refactor + in the future. E.g. use `eslint-disable-next-line` or `eslint-disable-line`. |