diff options
Diffstat (limited to 'doc/development')
| -rw-r--r-- | doc/development/contributing/style_guides.md | 4 | ||||
| -rw-r--r-- | doc/development/fe_guide/index.md | 11 | ||||
| -rw-r--r-- | doc/development/fe_guide/style/html.md | 53 | ||||
| -rw-r--r-- | doc/development/fe_guide/style/index.md | 21 | ||||
| -rw-r--r-- | doc/development/fe_guide/style/javascript.md | 303 | ||||
| -rw-r--r-- | doc/development/fe_guide/style/scss.md | 285 | ||||
| -rw-r--r-- | doc/development/fe_guide/style/vue.md | 418 | ||||
| -rw-r--r-- | doc/development/fe_guide/style_guide_js.md | 734 | ||||
| -rw-r--r-- | doc/development/fe_guide/style_guide_scss.md | 284 | ||||
| -rw-r--r-- | doc/development/fe_guide/tooling.md | 154 | ||||
| -rw-r--r-- | doc/development/fe_guide/vue.md | 2 | ||||
| -rw-r--r-- | doc/development/new_fe_guide/index.md | 4 | ||||
| -rw-r--r-- | doc/development/new_fe_guide/style/html.md | 56 | ||||
| -rw-r--r-- | doc/development/new_fe_guide/style/index.md | 18 | ||||
| -rw-r--r-- | doc/development/new_fe_guide/style/javascript.md | 198 | ||||
| -rw-r--r-- | doc/development/new_fe_guide/style/prettier.md | 101 | ||||
| -rw-r--r-- | doc/development/new_fe_guide/style/scss.md | 3 | ||||
| -rw-r--r-- | doc/development/new_fe_guide/style/vue.md | 3 |
18 files changed, 1268 insertions, 1384 deletions
diff --git a/doc/development/contributing/style_guides.md b/doc/development/contributing/style_guides.md index f825b3d7088..d53675cc716 100644 --- a/doc/development/contributing/style_guides.md +++ b/doc/development/contributing/style_guides.md @@ -40,8 +40,8 @@ This saves you time as you don't have to wait for the same errors to be detected [rss-source]: https://github.com/rubocop-hq/ruby-style-guide/blob/master/README.adoc#source-code-layout [rss-naming]: https://github.com/rubocop-hq/ruby-style-guide/blob/master/README.adoc#naming-conventions [doc-guidelines]: ../documentation/index.md "Documentation guidelines" -[js-styleguide]: ../fe_guide/style_guide_js.md "JavaScript styleguide" -[scss-styleguide]: ../fe_guide/style_guide_scss.md "SCSS styleguide" +[js-styleguide]: ../fe_guide/style/javascript.md "JavaScript styleguide" +[scss-styleguide]: ../fe_guide/style/scss.md "SCSS styleguide" [newlines-styleguide]: ../newlines_styleguide.md "Newlines styleguide" [testing]: ../testing_guide/index.md [us-english]: https://en.wikipedia.org/wiki/American_English diff --git a/doc/development/fe_guide/index.md b/doc/development/fe_guide/index.md index 1cf798cedb6..f13ef767660 100644 --- a/doc/development/fe_guide/index.md +++ b/doc/development/fe_guide/index.md @@ -76,15 +76,18 @@ Read the [frontend's FAQ](frontend_faq.md) for common small pieces of helpful in ## Style Guides -### [JavaScript Style Guide](style_guide_js.md) +See the relevant style guides for our guidelines and for information on linting: -We use eslint to enforce our JavaScript style guides. Our guide is based on +- [JavaScript](style/javascript.md). Our guide is based on the excellent [Airbnb][airbnb-js-style-guide] style guide with a few small changes. +- [SCSS](style/scss.md): our SCSS conventions which are enforced through [`scss-lint`](https://github.com/brigade/scss-lint). +- [HTML](style/html.md). Guidelines for writing HTML code consistent with the rest of the codebase. +- [Vue](style/vue.md). Guidelines and conventions for Vue code may be found here. -### [SCSS Style Guide](style_guide_scss.md) +## Tooling -Our SCSS conventions which are enforced through [scss-lint](https://github.com/sds/scss-lint). +Our code is automatically formatted with [Prettier](https://prettier.io) to follow our guidelines. Read our [Tooling guide](tooling.md) for more detail. ## [Performance](performance.md) diff --git a/doc/development/fe_guide/style/html.md b/doc/development/fe_guide/style/html.md new file mode 100644 index 00000000000..1445da3f0e1 --- /dev/null +++ b/doc/development/fe_guide/style/html.md @@ -0,0 +1,53 @@ +# HTML style guide + +## Buttons + +### Button type + +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> + +// good +<button type="button"></button> +``` + +### Button role + +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/Roles/button_role). + +```html +// bad +<div onClick="doSomething"></div> + +// good +<div role="button" onClick="doSomething"></div> +``` + +## Links + +### Blank target + +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> + +// good +<a href="url" target="_blank" rel="noopener noreferrer"></a> +``` + +### Fake links + +**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> +``` diff --git a/doc/development/fe_guide/style/index.md b/doc/development/fe_guide/style/index.md new file mode 100644 index 00000000000..3b07a8557f5 --- /dev/null +++ b/doc/development/fe_guide/style/index.md @@ -0,0 +1,21 @@ +# GitLab development style guides + +See below the relevant style guides, guidelines, linting, and other information for developing GitLab. + +## JavaScript style guide + +We use `eslint` to enforce our [JavaScript style guides](javascript.md). Our guide is based on +the excellent [AirBnB](https://github.com/airbnb/javascript) style guide with a few small +changes. + +## SCSS style guide + +Our [SCSS conventions](scss.md) which are enforced through [`scss-lint`](https://github.com/brigade/scss-lint). + +## HTML style guide + +Guidelines for writing [HTML code](html.md) consistent with the rest of the codebase. + +## Vue style guide + +Guidelines and conventions for Vue code may be found within the [Vue style guide](vue.md). diff --git a/doc/development/fe_guide/style/javascript.md b/doc/development/fe_guide/style/javascript.md new file mode 100644 index 00000000000..6921d418ea3 --- /dev/null +++ b/doc/development/fe_guide/style/javascript.md @@ -0,0 +1,303 @@ +--- +disqus_identifier: 'https://docs.gitlab.com/ee/development/fe_guide/style_guide_js.html' +--- + +# JavaScript style guide + +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. + +> **Tip:** +You can run eslint locally by running `yarn eslint` + +## 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. + +```javascript +// bad +class myClass { + constructor(config) { + this.config = config; + axios.get(this.config.endpoint) + } +} + +// good +class myClass { + constructor(config) { + this.config = config; + } + + makeRequest() { + axios.get(this.config.endpoint) + } +} +const instance = new myClass(); +instance.makeRequest(); +``` + +## 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. + +```javascript +// bad +class myClass { + constructor(config) { + this.config = config; + } + + init() { + document.addEventListener('click', () => {}); + } +} + +// good + +const myFunction = () => { + document.addEventListener('click', () => { + // handle callback here + }); +} +``` + +## Pass element container to constructor + +When your class manipulates the DOM, receive the element container as a parameter. +This is more maintainable and performant. + +```javascript +// bad +class a { + constructor() { + document.querySelector('.b'); + } +} + +// good +class a { + constructor(options) { + options.container.querySelector('.b'); + } +} +``` + +## Use ParseInt + +Use `ParseInt` when converting a numeric string into a number. + +```javascript +// bad +Number('10') + +// good +parseInt('10', 10); +``` + +## CSS Selectors - 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-`. + +```html +// bad +<button class="add-user"></button> + +// good +<button class="js-add-user"></button> +``` + +## ES Module Syntax + +Use ES module syntax to import modules: + +```javascript +// bad +const SomeClass = require('some_class'); + +// good +import SomeClass from 'some_class'; + +// bad +module.exports = SomeClass; + +// good +export default SomeClass; +``` + +_Note:_ We still use `require` in `scripts/` and `config/` files. + +## Absolute vs relative paths for modules + +Use relative paths if the module you are importing is less than two levels up. + +```javascript +// bad +import GitLabStyleGuide from '~/guides/GitLabStyleGuide'; + +// good +import GitLabStyleGuide from '../GitLabStyleGuide'; +``` + +If the module you are importing is two or more levels up, use an absolute path instead: + +```javascript +// bad +import GitLabStyleGuide from '../../../guides/GitLabStyleGuide'; + +// good +import GitLabStyleGuide from '~/GitLabStyleGuide'; +``` + +Additionally, **do not add to global namespace**. + +## 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. + +## Avoid XSS + +Do not use `innerHTML`, `append()` or `html()` to set content. It opens up too many +vulnerabilities. + +## ESLint + +ESLint behaviour can be found in our [tooling guide](../tooling.md). + +## IIFEs + +Avoid using IIFEs (Immediately-Invoked Function Expressions). Although +we have a lot of examples of files which wrap their contents in IIFEs, +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. + +## Global namespace and side effects + +Avoid adding to the global namespace. + +```javascript +// bad +window.MyClass = class { /* ... */ }; + +// good +export default class MyClass { /* ... */ } +``` + +Top-level side effects are forbidden in any script which contains `export`: + +```javascript +// bad +export default class MyClass { /* ... */ } + +document.addEventListener("DOMContentLoaded", function(event) { + new MyClass(); +} +``` + +Avoid constructors with side effects whenever possible. + +Side effects make constructors difficult to unit test and violate the [Single Responsibility Principle](https://en.wikipedia.org/wiki/Single_responsibility_principle). + +```javascript +// Bad +export class Foo { + constructor() { + this.currentState = state.INITIAL; + document.getElementById('root').addEventListener('click', this.handleCallback) + } + handleCallback() { + } +} + +// Good +export class Foo { + constructor() { + this.currentState = state.INITIAL; + } + initListener(element) { + element.addEventListener('click', this.handleCallback) + } + handleCallback() { + } +} +``` + +On the other hand, if a class only needs to extend a third-party or add +event listeners in some specific cases, they should be initialized outside +of the constructor. + +## Pure Functions and Data Mutation + +Strive to write many small pure functions and minimize where mutations occur + + ```javascript + // bad + const values = {foo: 1}; + + function impureFunction(items) { + const bar = 1; + + items.foo = items.a * bar + 2; + + return items.a; + } + + const c = impureFunction(values); + + // good + var values = {foo: 1}; + + function pureFunction (foo) { + var bar = 1; + + foo = foo * bar + 2; + + return foo; + } + + var c = pureFunction(values.foo); + ``` diff --git a/doc/development/fe_guide/style/scss.md b/doc/development/fe_guide/style/scss.md new file mode 100644 index 00000000000..02e7351d135 --- /dev/null +++ b/doc/development/fe_guide/style/scss.md @@ -0,0 +1,285 @@ +--- +disqus_identifier: 'https://docs.gitlab.com/ee/development/fe_guide/style_guide_scss.html' +--- + +# SCSS style guide + +This style guide recommends best practices for SCSS to make styles easy to read, +easy to maintain, and performant for the end-user. + +## Rules + +### Utility Classes + +As part of the effort for [cleaning up our CSS and moving our components into GitLab-UI](https://gitlab.com/groups/gitlab-org/-/epics/950) +led by the [GitLab UI WG](https://gitlab.com/gitlab-com/www-gitlab-com/merge_requests/20623) we prefer the use of utility classes over adding new CSS. However, complex CSS can be addressed by adding component classes. + +#### Where are utility classes defined? + +- [Bootstrap's Utility Classes](https://getbootstrap.com/docs/4.3/utilities/) +- [`common.scss`](https://gitlab.com/gitlab-org/gitlab/blob/master/app/assets/stylesheets/framework/common.scss) (old) +- [`utilities.scss`](https://gitlab.com/gitlab-org/gitlab/blob/master/app/assets/stylesheets/utilities.scss) (new) + +#### Where should I put new utility classes? + +New utility classes should be added to [`utilities.scss`](https://gitlab.com/gitlab-org/gitlab/blob/master/app/assets/stylesheets/utilities.scss). Existing classes include: + +| Name | Pattern | Example | +|------|---------|---------| +| Background color | `.bg-{variant}-{shade}` | `.bg-warning-400` | +| Text color | `.text-{variant}-{shade}` | `.text-success-500` | +| Font size | `.text-{size}` | `.text-2` | + +- `{variant}` is one of 'primary', 'secondary', 'success', 'warning', 'error' +- `{shade}` is one of the shades listed on [colors](https://design.gitlab.com/product-foundations/colors/) +- `{size}` is a number from 1-6 from our [Type scale](https://design.gitlab.com/product-foundations/typography/) + +#### When should I create component classes? + +We recommend a "utility-first" approach. + +1. Start with utility classes. +1. If composing utility classes into a component class removes code duplication and encapsulates a clear responsibility, do it. + +This encourages an organic growth of component classes and prevents the creation of one-off unreusable classes. Also, the kind of classes that emerge from "utility-first" tend to be design-centered (e.g. `.button`, `.alert`, `.card`) rather than domain-centered (e.g. `.security-report-widget`, `.commit-header-icon`). + +Examples of component classes that were created using "utility-first" include: + +- [`.circle-icon-container`](https://gitlab.com/gitlab-org/gitlab/blob/579fa8b8ec7eb38d40c96521f517c9dab8c3b97a/app/assets/stylesheets/framework/icons.scss#L85) +- [`.d-flex-center`](https://gitlab.com/gitlab-org/gitlab/blob/900083d89cd6af391d26ab7922b3f64fa2839bef/app/assets/stylesheets/framework/common.scss#L425) + +Inspiration: + +- <https://tailwindcss.com/docs/utility-first/> +- <https://tailwindcss.com/docs/extracting-components/> + +### Naming + +Filenames should use `snake_case`. + +CSS classes should use the `lowercase-hyphenated` format rather than +`snake_case` or `camelCase`. + +```scss +// Bad +.class_name { + color: #fff; +} + +// Bad +.className { + color: #fff; +} + +// Good +.class-name { + color: #fff; +} +``` + +### Formatting + +You should always use a space before a brace, braces should be on the same +line, each property should each get its own line, and there should be a space +between the property and its value. + +```scss +// Bad +.container-item { + width: 100px; height: 100px; + margin-top: 0; +} + +// Bad +.container-item +{ + width: 100px; + height: 100px; + margin-top: 0; +} + +// Bad +.container-item{ + width:100px; + height:100px; + margin-top:0; +} + +// Good +.container-item { + width: 100px; + height: 100px; + margin-top: 0; +} +``` + +Note that there is an exception for single-line rulesets, although these are +not typically recommended. + +```scss +p { margin: 0; padding: 0; } +``` + +### Colors + +HEX (hexadecimal) colors should use shorthand where possible, and should use +lower case letters to differentiate between letters and numbers, e.g. `#E3E3E3` +vs. `#e3e3e3`. + +```scss +// Bad +p { + color: #ffffff; +} + +// Bad +p { + color: #FFFFFF; +} + +// Good +p { + color: #fff; +} +``` + +### Indentation + +Indentation should always use two spaces for each indentation level. + +```scss +// Bad, four spaces +p { + color: #f00; +} + +// Good +p { + color: #f00; +} +``` + +### Semicolons + +Always include semicolons after every property. When the stylesheets are +minified, the semicolons will be removed automatically. + +```scss +// Bad +.container-item { + width: 100px; + height: 100px +} + +// Good +.container-item { + width: 100px; + height: 100px; +} +``` + +### Shorthand + +The shorthand form should be used for properties that support it. + +```scss +// Bad +margin: 10px 15px 10px 15px; +padding: 10px 10px 10px 10px; + +// Good +margin: 10px 15px; +padding: 10px; +``` + +### Zero Units + +Omit length units on zero values, they're unnecessary and not including them +is slightly more performant. + +```scss +// Bad +.item-with-padding { + padding: 0px; +} + +// Good +.item-with-padding { + padding: 0; +} +``` + +### Selectors with a `js-` Prefix + +Do not use any selector prefixed with `js-` for styling purposes. These +selectors are intended for use only with JavaScript to allow for removal or +renaming without breaking styling. + +### IDs + +Don't use ID selectors in CSS. + +```scss +// Bad +#my-element { + padding: 0; +} + +// Good +.my-element { + padding: 0; +} +``` + +### Variables + +Before adding a new variable for a color or a size, guarantee: + +- There isn't already one +- There isn't a similar one we can use instead. + +## Linting + +We use [SCSS Lint](https://github.com/sds/scss-lint) to check for style guide conformity. It uses the +ruleset in `.scss-lint.yml`, which is located in the home directory of the +project. + +To check if any warnings will be produced by your changes, you can run `rake +scss_lint` in the GitLab directory. SCSS Lint will also run in GitLab CI to +catch any warnings. + +If the Rake task is throwing warnings you don't understand, SCSS Lint's +documentation includes [a full list of their linters][scss-lint-documentation](https://github.com/sds/scss-lint/blob/master/lib/scss_lint/linter/README.md). + +### Fixing issues + +If you want to automate changing a large portion of the codebase to conform to +the SCSS style guide, you can use [CSSComb][csscomb]. First install +[Node][node] and [NPM][npm], then run `npm install csscomb -g` to install +CSSComb globally (system-wide). Run it in the GitLab directory with +`csscomb app/assets/stylesheets` to automatically fix issues with CSS/SCSS. + +Note that this won't fix every problem, but it should fix a majority. + +### Ignoring issues + +If you want a line or set of lines to be ignored by the linter, you can use +`// scss-lint:disable RuleName` ([more info](https://github.com/sds/scss-lint#disabling-linters-via-source)): + +```scss +// This lint rule is disabled because it is supported only in Chrome/Safari +// scss-lint:disable PropertySpelling +body { + text-decoration-skip: ink; +} +// scss-lint:enable PropertySpelling +``` + +Make sure a comment is added on the line above the `disable` rule, otherwise the +linter will throw a warning. `DisableLinterReason` is enabled to make sure the +style guide isn't being ignored, and to communicate to others why the style +guide is ignored in this instance. + +[csscomb]: https://github.com/csscomb/csscomb.js +[node]: https://github.com/nodejs/node +[npm]: https://www.npmjs.com/ diff --git a/doc/development/fe_guide/style/vue.md b/doc/development/fe_guide/style/vue.md new file mode 100644 index 00000000000..2499623e66a --- /dev/null +++ b/doc/development/fe_guide/style/vue.md @@ -0,0 +1,418 @@ +# Vue.js style guide + +## Linting + +We default to [eslint-vue-plugin](https://github.com/vuejs/eslint-plugin-vue), with the `plugin:vue/recommended`. +Please check this [rules](https://github.com/vuejs/eslint-plugin-vue#bulb-rules) for more documentation. + +## Basic Rules + +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 + // bad + class { + init() { + new Component({}) + } + } + + // good + document.addEventListener('DOMContentLoaded', () => new Vue({ + el: '#element', + components: { + componentName + }, + render: createElement => createElement('component-name'), + })); + ``` + +1. Do not use a singleton for the service or the store + + ```javascript + // bad + class Store { + constructor() { + if (!this.prototype.singleton) { + // do something + } + } + } + + // good + class Store { + constructor() { + // do something + } + } + ``` + +1. Use `.vue` for Vue templates. Do not use `%template` in HAML. + +## Naming + +1. **Extensions**: Use `.vue` extension for Vue components. Do not use `.js` as file extension ([#34371]). +1. **Reference Naming**: Use PascalCase for their instances: + + ```javascript + // bad + import cardBoard from 'cardBoard.vue' + + components: { + cardBoard, + }; + + // good + import CardBoard from 'cardBoard.vue' + + components: { + CardBoard, + }; + ``` + +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"> + + // good + <component css-class="btn"> + + // bad + <component myProp="prop" /> + + // good + <component my-prop="prop" /> + ``` + +[#34371]: https://gitlab.com/gitlab-org/gitlab-foss/issues/34371 + +## Alignment + +1. Follow these alignment styles for the template method: + + 1. With more than one attribute, all attributes should be on a new line: + + ```javascript + // bad + <component v-if="bar" + param="baz" /> + + <button class="btn">Click me</button> + + // good + <component + v-if="bar" + param="baz" + /> + + <button class="btn"> + Click me + </button> + ``` + + 1. The tag can be inline if there is only one attribute: + + ```javascript + // good + <component bar="bar" /> + + // good + <component + bar="bar" + /> + + // bad + <component + bar="bar" /> + ``` + +## Quotes + +1. Always use double quotes `"` inside templates and single quotes `'` for all other JS. + + ```javascript + // bad + template: ` + <button :class='style'>Button</button> + ` + + // good + template: ` + <button :class="style">Button</button> + ` + ``` + +## Props + +1. Props should be declared as an object + + ```javascript + // bad + props: ['foo'] + + // good + props: { + foo: { + type: String, + required: false, + default: 'bar' + } + } + ``` + +1. Required key should always be provided when declaring a prop + + ```javascript + // bad + props: { + foo: { + type: String, + } + } + + // good + props: { + foo: { + type: String, + required: false, + default: 'bar' + } + } + ``` + +1. Default key should be provided if the prop is not required. + _Note:_ There are some scenarios where we need to check for the existence of the property. + On those a default key should not be provided. + + ```javascript + // good + props: { + foo: { + type: String, + required: false, + } + } + + // good + props: { + foo: { + type: String, + required: false, + default: 'bar' + } + } + + // good + props: { + foo: { + type: String, + required: true + } + } + ``` + +## Data + +1. `data` method should always be a function + + ```javascript + // bad + data: { + foo: 'foo' + } + + // good + data() { + return { + foo: 'foo' + }; + } + ``` + +## Directives + +1. Shorthand `@` is preferable over `v-on` + + ```javascript + // bad + <component v-on:click="eventHandler"/> + + // good + <component @click="eventHandler"/> + ``` + +1. Shorthand `:` is preferable over `v-bind` + + ```javascript + // bad + <component v-bind:class="btn"/> + + // good + <component :class="btn"/> + ``` + +1. Shorthand `#` is preferable over `v-slot` + + ```javascript + // bad + <template v-slot:header></template> + + // good + <template #header></template> + ``` + +## Closing tags + +1. Prefer self closing component tags + + ```javascript + // bad + <component></component> + + // good + <component /> + ``` + +## Component usage within templates + +1. Prefer a component's kebab-cased name over other styles when using it in a template + + ```javascript + // bad + <MyComponent /> + + // good + <my-component /> + ``` + +## Ordering + +1. Tag order in `.vue` file + + ``` + <script> + // ... + </script> + + <template> + // ... + </template> + + // We don't use scoped styles but there are few instances of this + <style> + // ... + </style> + ``` + +1. Properties in a Vue Component: + Check [order of properties in components rule](https://github.com/vuejs/eslint-plugin-vue/blob/master/docs/rules/order-in-components.md). + +## `:key` + +When using `v-for` you need to provide a *unique* `:key` attribute for each item. + +1. If the elements of the array being iterated have an unique `id` it is advised to use it: + + ```html + <div + v-for="item in items" + :key="item.id" + > + <!-- content --> + </div> + ``` + +1. When the elements being iterated don't have a unique id, you can use the array index as the `:key` attribute + + ```html + <div + v-for="(item, index) in items" + :key="index" + > + <!-- content --> + </div> + ``` + +1. When using `v-for` with `template` and there is more than one child element, the `:key` values must be unique. It's advised to use `kebab-case` namespaces. + + ```html + <template v-for="(item, index) in items"> + <span :key="`span-${index}`"></span> + <button :key="`button-${index}`"></button> + </template> + ``` + +1. When dealing with nested `v-for` use the same guidelines as above. + + ```html + <div + v-for="item in items" + :key="item.id" + > + <span + v-for="element in array" + :key="element.id" + > + <!-- content --> + </span> + </div> + ``` + +Useful links: + +1. [`key`](https://vuejs.org/v2/guide/list.html#key) +1. [Vue Style Guide: Keyed v-for](https://vuejs.org/v2/style-guide/#Keyed-v-for-essential ) + +## Vue and Bootstrap + +1. Tooltips: Do not rely on `has-tooltip` class name for Vue components + + ```javascript + // bad + <span + class="has-tooltip" + title="Some tooltip text"> + Text + </span> + + // good + <span + v-tooltip + title="Some tooltip text"> + Text + </span> + ``` + +1. Tooltips: When using a tooltip, include the tooltip directive, `./app/assets/javascripts/vue_shared/directives/tooltip.js` + +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'); + ``` + +## The JavaScript/Vue Accord + +The goal of this accord is to make sure we are all on the same page. + +1. When writing Vue, you may not use jQuery in your application. + 1. If you need to grab data from the DOM, you may query the DOM 1 time while bootstrapping your application to grab data attributes using `dataset`. You can do this without jQuery. + 1. You may use a jQuery dependency in Vue.js following [this example from the docs](https://vuejs.org/v2/examples/select2.html). + 1. If an outside jQuery Event needs to be listen to inside the Vue application, you may use jQuery event listeners. + 1. We will avoid adding new jQuery events when they are not required. Instead of adding new jQuery events take a look at [different methods to do the same task](https://vuejs.org/v2/api/#vm-emit). +1. You may query the `window` object 1 time, while bootstrapping your application for application specific data (e.g. `scrollTo` is ok to access anytime). Do this access during the bootstrapping of your application. +1. You may have a temporary but immediate need to create technical debt by writing code that does not follow our standards, to be refactored later. Maintainers need to be ok with the tech debt in the first place. An issue should be created for that tech debt to evaluate it further and discuss. In the coming months you should fix that tech debt, with it's priority to be determined by maintainers. +1. When creating tech debt you must write the tests for that code before hand and those tests may not be rewritten. e.g. jQuery tests rewritten to Vue tests. +1. You may choose to use VueX as a centralized state management. If you choose not to use VueX, you must use the *store pattern* which can be found in the [Vue.js documentation](https://vuejs.org/v2/guide/state-management.html#Simple-State-Management-from-Scratch). +1. Once you have chosen a centralized state management solution you must use it for your entire application. i.e. Don't mix and match your state management solutions. diff --git a/doc/development/fe_guide/style_guide_js.md b/doc/development/fe_guide/style_guide_js.md index 43cd8180b6e..f3fa80325ef 100644 --- a/doc/development/fe_guide/style_guide_js.md +++ b/doc/development/fe_guide/style_guide_js.md @@ -1,731 +1,5 @@ -# Style guides and linting +--- +redirect_to: 'style/javascript.md' +--- -See the relevant style guides for our guidelines and for information on linting: - -## JavaScript - -We defer to [Airbnb][airbnb-js-style-guide] on most style-related -conventions and enforce them with eslint. - -See [our current .eslintrc](https://gitlab.com/gitlab-org/gitlab/blob/master/.eslintrc.yml) for specific rules and patterns. - -### Common - -#### ESlint - -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 */ - - // better - /* eslint-disable some-rule, some-other-rule */ - - // best - // nothing :) - ``` - -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 */ - - import Foo from 'foo'; - - new Foo(); - - // better - import Foo from '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](https://eslint.org/docs/rules/no-new) - 1. [class-methods-use-this](https://eslint.org/docs/rules/class-methods-use-this) - -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'; - - // good - /* eslint-disable no-new */ - /* global Foo */ - - import Bar from './bar'; - ``` - -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 */ - - // good - /* global Flash */ - /* global Cookies */ - /* global jQuery */ - ``` - -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) {} - - // good - fn(options) {} - ``` - -#### Modules, Imports, and Exports - -1. Use ES module syntax to import modules - - ```javascript - // bad - const SomeClass = require('some_class'); - - // good - import SomeClass from 'some_class'; - - // bad - module.exports = SomeClass; - - // good - export default SomeClass; - ``` - - Import statements are following usual naming guidelines, for example object literals use camel case: - - ```javascript - // some_object file - export default { - key: 'value', - }; - - // bad - import ObjectLiteral from 'some_object'; - - // good - import objectLiteral from 'some_object'; - ``` - -1. Relative paths: when importing a module in the same directory, a child - directory, or an immediate parent directory prefer relative paths. When - importing a module which is two or more levels up, prefer either `~/` or `ee/`. - - In **app/assets/javascripts/my-feature/subdir**: - - ```javascript - // bad - import Foo from '~/my-feature/foo'; - import Bar from '~/my-feature/subdir/bar'; - import Bin from '~/my-feature/subdir/lib/bin'; - - // good - import Foo from '../foo'; - import Bar from './bar'; - import Bin from './lib/bin'; - ``` - - In **spec/javascripts**: - - ```javascript - // bad - import Foo from '../../app/assets/javascripts/my-feature/foo'; - - // good - import Foo from '~/my-feature/foo'; - ``` - - When referencing an **EE component**: - - ```javascript - // bad - import Foo from '../../../../../ee/app/assets/javascripts/my-feature/ee-foo'; - - // good - import Foo from 'ee/my-feature/foo'; - ``` - -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 { /* ... */ }; - - // good - export default class MyClass { /* ... */ } - ``` - -1. Side effects are forbidden in any script which contains export - - ```javascript - // bad - export default class MyClass { /* ... */ } - - document.addEventListener("DOMContentLoaded", function(event) { - new MyClass(); - } - ``` - -#### Data Mutation and Pure functions - -1. Strive to write many small pure functions, and minimize where mutations occur. - - ```javascript - // bad - const values = {foo: 1}; - - function impureFunction(items) { - const bar = 1; - - items.foo = items.a * bar + 2; - - return items.a; - } - - const c = impureFunction(values); - - // good - var values = {foo: 1}; - - function pureFunction (foo) { - var bar = 1; - - foo = foo * bar + 2; - - return foo; - } - - var c = pureFunction(values.foo); - ``` - -1. Avoid constructors with side-effects. - Although we aim for code without side-effects we need some side-effects for our code to run. - - If the class won't do anything if we only instantiate it, it's ok to add side effects into the constructor (_Note:_ The following is just an example. If the only purpose of the class is to add an event listener and handle the callback a function will be more suitable.) - - ```javascript - // Bad - export class Foo { - constructor() { - this.init(); - } - init() { - document.addEventListener('click', this.handleCallback) - }, - handleCallback() { - - } - } - - // Good - export class Foo { - constructor() { - document.addEventListener() - } - handleCallback() { - } - } - ``` - - On the other hand, if a class only needs to extend a third party/add event listeners in some specific cases, they should be initialized outside of the constructor. - -1. Prefer `.map`, `.reduce` or `.filter` over `.forEach` - A forEach will most likely cause side effects, it will be mutating the array being iterated. Prefer using `.map`, - `.reduce` or `.filter` - - ```javascript - const users = [ { name: 'Foo' }, { name: 'Bar' } ]; - - // bad - users.forEach((user, index) => { - user.id = index; - }); - - // good - const usersWithId = users.map((user, index) => { - return Object.assign({}, user, { id: index }); - }); - ``` - -#### Parse Strings into Numbers - -1. `parseInt()` is preferable over `Number()` or `+` - - ```javascript - // bad - +'10' // 10 - - // good - Number('10') // 10 - - // better - parseInt('10', 10); - ``` - -#### CSS classes used for JavaScript - -1. If the class is being used in JavaScript it needs to be prepend with `js-` - - ```html - // bad - <button class="add-user"> - Add User - </button> - - // good - <button class="js-add-user"> - Add User - </button> - ``` - -### Vue.js - -#### `eslint-vue-plugin` - -We default to [eslint-vue-plugin][eslint-plugin-vue], with the `plugin:vue/recommended`. -Please check this [rules][eslint-plugin-vue-rules] for more documentation. - -#### Basic Rules - -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 - // bad - class { - init() { - new Component({}) - } - } - - // good - document.addEventListener('DOMContentLoaded', () => new Vue({ - el: '#element', - components: { - componentName - }, - render: createElement => createElement('component-name'), - })); - ``` - -1. Do not use a singleton for the service or the store - - ```javascript - // bad - class Store { - constructor() { - if (!this.prototype.singleton) { - // do something - } - } - } - - // good - class Store { - constructor() { - // do something - } - } - ``` - -1. Use `.vue` for Vue templates. Do not use `%template` in HAML. - -#### Naming - -1. **Extensions**: Use `.vue` extension for Vue components. Do not use `.js` as file extension ([#34371]). -1. **Reference Naming**: Use PascalCase for their instances: - - ```javascript - // bad - import cardBoard from 'cardBoard.vue' - - components: { - cardBoard, - }; - - // good - import CardBoard from 'cardBoard.vue' - - components: { - CardBoard, - }; - ``` - -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"> - - // good - <component css-class="btn"> - - // bad - <component myProp="prop" /> - - // good - <component my-prop="prop" /> - ``` - -[#34371]: https://gitlab.com/gitlab-org/gitlab-foss/issues/34371 - -#### Alignment - -1. Follow these alignment styles for the template method: - - 1. With more than one attribute, all attributes should be on a new line: - - ```javascript - // bad - <component v-if="bar" - param="baz" /> - - <button class="btn">Click me</button> - - // good - <component - v-if="bar" - param="baz" - /> - - <button class="btn"> - Click me - </button> - ``` - - 1. The tag can be inline if there is only one attribute: - - ```javascript - // good - <component bar="bar" /> - - // good - <component - bar="bar" - /> - - // bad - <component - bar="bar" /> - ``` - -#### Quotes - -1. Always use double quotes `"` inside templates and single quotes `'` for all other JS. - - ```javascript - // bad - template: ` - <button :class='style'>Button</button> - ` - - // good - template: ` - <button :class="style">Button</button> - ` - ``` - -#### Props - -1. Props should be declared as an object - - ```javascript - // bad - props: ['foo'] - - // good - props: { - foo: { - type: String, - required: false, - default: 'bar' - } - } - ``` - -1. Required key should always be provided when declaring a prop - - ```javascript - // bad - props: { - foo: { - type: String, - } - } - - // good - props: { - foo: { - type: String, - required: false, - default: 'bar' - } - } - ``` - -1. Default key should be provided if the prop is not required. - _Note:_ There are some scenarios where we need to check for the existence of the property. - On those a default key should not be provided. - - ```javascript - // good - props: { - foo: { - type: String, - required: false, - } - } - - // good - props: { - foo: { - type: String, - required: false, - default: 'bar' - } - } - - // good - props: { - foo: { - type: String, - required: true - } - } - ``` - -#### Data - -1. `data` method should always be a function - - ```javascript - // bad - data: { - foo: 'foo' - } - - // good - data() { - return { - foo: 'foo' - }; - } - ``` - -#### Directives - -1. Shorthand `@` is preferable over `v-on` - - ```javascript - // bad - <component v-on:click="eventHandler"/> - - // good - <component @click="eventHandler"/> - ``` - -1. Shorthand `:` is preferable over `v-bind` - - ```javascript - // bad - <component v-bind:class="btn"/> - - // good - <component :class="btn"/> - ``` - -1. Shorthand `#` is preferable over `v-slot` - - ```javascript - // bad - <template v-slot:header></template> - - // good - <template #header></template> - ``` - -#### Closing tags - -1. Prefer self closing component tags - - ```javascript - // bad - <component></component> - - // good - <component /> - ``` - -#### Component usage within templates - -1. Prefer a component's kebab-cased name over other styles when using it in a template - - ```javascript - // bad - <MyComponent /> - - // good - <my-component /> - ``` - -#### Ordering - -1. Tag order in `.vue` file - - ``` - <script> - // ... - </script> - - <template> - // ... - </template> - - // We don't use scoped styles but there are few instances of this - <style> - // ... - </style> - ``` - -1. Properties in a Vue Component: - Check [order of properties in components rule][vue-order]. - -#### `:key` - -When using `v-for` you need to provide a *unique* `:key` attribute for each item. - -1. If the elements of the array being iterated have an unique `id` it is advised to use it: - - ```html - <div - v-for="item in items" - :key="item.id" - > - <!-- content --> - </div> - ``` - -1. When the elements being iterated don't have a unique id, you can use the array index as the `:key` attribute - - ```html - <div - v-for="(item, index) in items" - :key="index" - > - <!-- content --> - </div> - ``` - -1. When using `v-for` with `template` and there is more than one child element, the `:key` values must be unique. It's advised to use `kebab-case` namespaces. - - ```html - <template v-for="(item, index) in items"> - <span :key="`span-${index}`"></span> - <button :key="`button-${index}`"></button> - </template> - ``` - -1. When dealing with nested `v-for` use the same guidelines as above. - - ```html - <div - v-for="item in items" - :key="item.id" - > - <span - v-for="element in array" - :key="element.id" - > - <!-- content --> - </span> - </div> - ``` - -Useful links: - -1. [`key`](https://vuejs.org/v2/guide/list.html#key) -1. [Vue Style Guide: Keyed v-for](https://vuejs.org/v2/style-guide/#Keyed-v-for-essential ) - -#### Vue and Bootstrap - -1. Tooltips: Do not rely on `has-tooltip` class name for Vue components - - ```javascript - // bad - <span - class="has-tooltip" - title="Some tooltip text"> - Text - </span> - - // good - <span - v-tooltip - title="Some tooltip text"> - Text - </span> - ``` - -1. Tooltips: When using a tooltip, include the tooltip directive, `./app/assets/javascripts/vue_shared/directives/tooltip.js` - -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'); - ``` - -### The JavaScript/Vue Accord - -The goal of this accord is to make sure we are all on the same page. - -1. When writing Vue, you may not use jQuery in your application. - 1. If you need to grab data from the DOM, you may query the DOM 1 time while bootstrapping your application to grab data attributes using `dataset`. You can do this without jQuery. - 1. You may use a jQuery dependency in Vue.js following [this example from the docs](https://vuejs.org/v2/examples/select2.html). - 1. If an outside jQuery Event needs to be listen to inside the Vue application, you may use jQuery event listeners. - 1. We will avoid adding new jQuery events when they are not required. Instead of adding new jQuery events take a look at [different methods to do the same task](https://vuejs.org/v2/api/#vm-emit). -1. You may query the `window` object 1 time, while bootstrapping your application for application specific data (e.g. `scrollTo` is ok to access anytime). Do this access during the bootstrapping of your application. -1. You may have a temporary but immediate need to create technical debt by writing code that does not follow our standards, to be refactored later. Maintainers need to be ok with the tech debt in the first place. An issue should be created for that tech debt to evaluate it further and discuss. In the coming months you should fix that tech debt, with it's priority to be determined by maintainers. -1. When creating tech debt you must write the tests for that code before hand and those tests may not be rewritten. e.g. jQuery tests rewritten to Vue tests. -1. You may choose to use VueX as a centralized state management. If you choose not to use VueX, you must use the *store pattern* which can be found in the [Vue.js documentation](https://vuejs.org/v2/guide/state-management.html#Simple-State-Management-from-Scratch). -1. Once you have chosen a centralized state management solution you must use it for your entire application. i.e. Don't mix and match your state management solutions. - -## SCSS - -- [SCSS](style_guide_scss.md) - -[airbnb-js-style-guide]: https://github.com/airbnb/javascript -[eslintrc]: https://gitlab.com/gitlab-org/gitlab/blob/master/.eslintrc -[eslint-plugin-vue]: https://github.com/vuejs/eslint-plugin-vue -[eslint-plugin-vue-rules]: https://github.com/vuejs/eslint-plugin-vue#bulb-rules -[vue-order]: https://github.com/vuejs/eslint-plugin-vue/blob/master/docs/rules/order-in-components.md +This document was moved to [another location](style/javascript.md). diff --git a/doc/development/fe_guide/style_guide_scss.md b/doc/development/fe_guide/style_guide_scss.md index 1a5b24ce6f4..2b4e6427a18 100644 --- a/doc/development/fe_guide/style_guide_scss.md +++ b/doc/development/fe_guide/style_guide_scss.md @@ -1,281 +1,5 @@ -# SCSS styleguide +--- +redirect_to: 'style/scss.md' +--- -This style guide recommends best practices for SCSS to make styles easy to read, -easy to maintain, and performant for the end-user. - -## Rules - -### Utility Classes - -As part of the effort for [cleaning up our CSS and moving our components into GitLab-UI](https://gitlab.com/groups/gitlab-org/-/epics/950) -led by the [GitLab UI WG](https://gitlab.com/gitlab-com/www-gitlab-com/merge_requests/20623) we prefer the use of utility classes over adding new CSS. However, complex CSS can be addressed by adding component classes. - -#### Where are utility classes defined? - -- [Bootstrap's Utility Classes](https://getbootstrap.com/docs/4.3/utilities/) -- [`common.scss`](https://gitlab.com/gitlab-org/gitlab/blob/master/app/assets/stylesheets/framework/common.scss) (old) -- [`utilities.scss`](https://gitlab.com/gitlab-org/gitlab/blob/master/app/assets/stylesheets/utilities.scss) (new) - -#### Where should I put new utility classes? - -New utility classes should be added to [`utilities.scss`](https://gitlab.com/gitlab-org/gitlab/blob/master/app/assets/stylesheets/utilities.scss). Existing classes include: - -| Name | Pattern | Example | -|------|---------|---------| -| Background color | `.bg-{variant}-{shade}` | `.bg-warning-400` | -| Text color | `.text-{variant}-{shade}` | `.text-success-500` | -| Font size | `.text-{size}` | `.text-2` | - -- `{variant}` is one of 'primary', 'secondary', 'success', 'warning', 'error' -- `{shade}` is one of the shades listed on [colors](https://design.gitlab.com/product-foundations/colors/) -- `{size}` is a number from 1-6 from our [Type scale](https://design.gitlab.com/product-foundations/typography/) - -#### When should I create component classes? - -We recommend a "utility-first" approach. - -1. Start with utility classes. -1. If composing utility classes into a component class removes code duplication and encapsulates a clear responsibility, do it. - -This encourages an organic growth of component classes and prevents the creation of one-off unreusable classes. Also, the kind of classes that emerge from "utility-first" tend to be design-centered (e.g. `.button`, `.alert`, `.card`) rather than domain-centered (e.g. `.security-report-widget`, `.commit-header-icon`). - -Examples of component classes that were created using "utility-first" include: - -- [`.circle-icon-container`](https://gitlab.com/gitlab-org/gitlab/blob/579fa8b8ec7eb38d40c96521f517c9dab8c3b97a/app/assets/stylesheets/framework/icons.scss#L85) -- [`.d-flex-center`](https://gitlab.com/gitlab-org/gitlab/blob/900083d89cd6af391d26ab7922b3f64fa2839bef/app/assets/stylesheets/framework/common.scss#L425) - -Inspiration: - -- <https://tailwindcss.com/docs/utility-first/> -- <https://tailwindcss.com/docs/extracting-components/> - -### Naming - -Filenames should use `snake_case`. - -CSS classes should use the `lowercase-hyphenated` format rather than -`snake_case` or `camelCase`. - -```scss -// Bad -.class_name { - color: #fff; -} - -// Bad -.className { - color: #fff; -} - -// Good -.class-name { - color: #fff; -} -``` - -### Formatting - -You should always use a space before a brace, braces should be on the same -line, each property should each get its own line, and there should be a space -between the property and its value. - -```scss -// Bad -.container-item { - width: 100px; height: 100px; - margin-top: 0; -} - -// Bad -.container-item -{ - width: 100px; - height: 100px; - margin-top: 0; -} - -// Bad -.container-item{ - width:100px; - height:100px; - margin-top:0; -} - -// Good -.container-item { - width: 100px; - height: 100px; - margin-top: 0; -} -``` - -Note that there is an exception for single-line rulesets, although these are -not typically recommended. - -```scss -p { margin: 0; padding: 0; } -``` - -### Colors - -HEX (hexadecimal) colors should use shorthand where possible, and should use -lower case letters to differentiate between letters and numbers, e.g. `#E3E3E3` -vs. `#e3e3e3`. - -```scss -// Bad -p { - color: #ffffff; -} - -// Bad -p { - color: #FFFFFF; -} - -// Good -p { - color: #fff; -} -``` - -### Indentation - -Indentation should always use two spaces for each indentation level. - -```scss -// Bad, four spaces -p { - color: #f00; -} - -// Good -p { - color: #f00; -} -``` - -### Semicolons - -Always include semicolons after every property. When the stylesheets are -minified, the semicolons will be removed automatically. - -```scss -// Bad -.container-item { - width: 100px; - height: 100px -} - -// Good -.container-item { - width: 100px; - height: 100px; -} -``` - -### Shorthand - -The shorthand form should be used for properties that support it. - -```scss -// Bad -margin: 10px 15px 10px 15px; -padding: 10px 10px 10px 10px; - -// Good -margin: 10px 15px; -padding: 10px; -``` - -### Zero Units - -Omit length units on zero values, they're unnecessary and not including them -is slightly more performant. - -```scss -// Bad -.item-with-padding { - padding: 0px; -} - -// Good -.item-with-padding { - padding: 0; -} -``` - -### Selectors with a `js-` Prefix - -Do not use any selector prefixed with `js-` for styling purposes. These -selectors are intended for use only with JavaScript to allow for removal or -renaming without breaking styling. - -### IDs - -Don't use ID selectors in CSS. - -```scss -// Bad -#my-element { - padding: 0; -} - -// Good -.my-element { - padding: 0; -} -``` - -### Variables - -Before adding a new variable for a color or a size, guarantee: - -- There isn't already one -- There isn't a similar one we can use instead. - -## Linting - -We use [SCSS Lint](https://github.com/sds/scss-lint) to check for style guide conformity. It uses the -ruleset in `.scss-lint.yml`, which is located in the home directory of the -project. - -To check if any warnings will be produced by your changes, you can run `rake -scss_lint` in the GitLab directory. SCSS Lint will also run in GitLab CI to -catch any warnings. - -If the Rake task is throwing warnings you don't understand, SCSS Lint's -documentation includes [a full list of their linters][scss-lint-documentation](https://github.com/sds/scss-lint/blob/master/lib/scss_lint/linter/README.md). - -### Fixing issues - -If you want to automate changing a large portion of the codebase to conform to -the SCSS style guide, you can use [CSSComb][csscomb]. First install -[Node][node] and [NPM][npm], then run `npm install csscomb -g` to install -CSSComb globally (system-wide). Run it in the GitLab directory with -`csscomb app/assets/stylesheets` to automatically fix issues with CSS/SCSS. - -Note that this won't fix every problem, but it should fix a majority. - -### Ignoring issues - -If you want a line or set of lines to be ignored by the linter, you can use -`// scss-lint:disable RuleName` ([more info](https://github.com/sds/scss-lint#disabling-linters-via-source)): - -```scss -// This lint rule is disabled because it is supported only in Chrome/Safari -// scss-lint:disable PropertySpelling -body { - text-decoration-skip: ink; -} -// scss-lint:enable PropertySpelling -``` - -Make sure a comment is added on the line above the `disable` rule, otherwise the -linter will throw a warning. `DisableLinterReason` is enabled to make sure the -style guide isn't being ignored, and to communicate to others why the style -guide is ignored in this instance. - -[csscomb]: https://github.com/csscomb/csscomb.js -[node]: https://github.com/nodejs/node -[npm]: https://www.npmjs.com/ +This document was moved to [another location](style/scss.md). diff --git a/doc/development/fe_guide/tooling.md b/doc/development/fe_guide/tooling.md new file mode 100644 index 00000000000..ec5e094cfa6 --- /dev/null +++ b/doc/development/fe_guide/tooling.md @@ -0,0 +1,154 @@ +# Tooling + +## ESLint + +We use ESLint to encapsulate and enforce frontend code standards. Our configuration may be found in the [gitlab-eslint-config](https://gitlab.com/gitlab-org/gitlab-eslint-config) project. + +### 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. + +Do not disable specific ESLint rules. To avoid introducing technical debt, you may disable the following +rules only if you are invoking/instantiating existing code modules. + +- [`no-new`](https://eslint.org/docs/rules/no-new) +- [`class-method-use-this`](https://eslint.org/docs/rules/class-methods-use-this) + +NOTE: **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`. + +### Disabling ESLint for a single violation + +If you do need to disable a rule for a single violation, disable it for the smallest amount of code necessary: + +```javascript +// bad +/* eslint-disable no-new */ + +import Foo from 'foo'; + +new Foo(); + +// better +import Foo from 'foo'; + +// eslint-disable-next-line no-new +new Foo(); +``` + +### The `no-undef` rule and declaring globals + +**Never** disable the `no-undef` rule. Declare globals with `/* global Foo */` instead. + +When declaring multiple globals, always use one `/* global [name] */` line per variable. + +```javascript +// bad +/* globals Flash, Cookies, jQuery */ + +// good +/* global Flash */ +/* global Cookies */ +/* global jQuery */ +``` + +## Formatting with Prettier + +Our code is automatically formatted with [Prettier](https://prettier.io) to follow our style guides. Prettier is taking care of formatting .js, .vue, and .scss files based on the standard prettier rules. You can find all settings for Prettier in `.prettierrc`. + +### Editor + +The easiest way to include prettier in your workflow is by setting up your preferred editor (all major editors are supported) accordingly. We suggest setting up prettier to run automatically when each file is saved. Find [here](https://prettier.io/docs/en/editors.html) the best way to set it up in your preferred editor. + +Please take care that you only let Prettier format the same file types as the global Yarn script does (.js, .vue, and .scss). In VSCode by example you can easily exclude file formats in your settings file: + +``` + "prettier.disableLanguages": [ + "json", + "markdown" + ], +``` + +### Yarn Script + +The following yarn scripts are available to do global formatting: + +``` +yarn prettier-staged-save +``` + +Updates all currently staged files (based on `git diff`) with Prettier and saves the needed changes. + +``` +yarn prettier-staged +``` + +Checks all currently staged files (based on `git diff`) with Prettier and log which files would need manual updating to the console. + +``` +yarn prettier-all +``` + +Checks all files with Prettier and logs which files need manual updating to the console. + +``` +yarn prettier-all-save +``` + +Formats all files in the repository with Prettier. (This should only be used to test global rule updates otherwise you would end up with huge MR's). + +The source of these Yarn scripts can be found in `/scripts/frontend/prettier.js`. + +#### Scripts during Conversion period + +``` +node ./scripts/frontend/prettier.js check-all ./vendor/ +``` + +This will go over all files in a specific folder check it. + +``` +node ./scripts/frontend/prettier.js save-all ./vendor/ +``` + +This will go over all files in a specific folder and save it. + +### VSCode Settings + +#### Select Prettier as default formatter + +To select Prettier as a formatter, add the following properties to your User or Workspace Settings: + +```javascript +{ + "[html]": { + "editor.defaultFormatter": "esbenp.prettier-vscode" + }, + "[javascript]": { + "editor.defaultFormatter": "esbenp.prettier-vscode" + }, + "[vue]": { + "editor.defaultFormatter": "esbenp.prettier-vscode" + } +} +``` + +#### Format on Save + +To automatically format your files with Prettier, add the following properties to your User or Workspace Settings: + +```javascript +{ + "[html]": { + "editor.formatOnSave": true + }, + "[javascript]": { + "editor.formatOnSave": true + }, + "[vue]": { + "editor.formatOnSave": true + }, +} +``` diff --git a/doc/development/fe_guide/vue.md b/doc/development/fe_guide/vue.md index e13a7d1e478..c3edaec70c7 100644 --- a/doc/development/fe_guide/vue.md +++ b/doc/development/fe_guide/vue.md @@ -179,7 +179,7 @@ Check this [page](vuex.md) for more details. ## Style guide -Please refer to the Vue section of our [style guide](style_guide_js.md#vuejs) +Please refer to the Vue section of our [style guide](style/vue.md) for best practices while writing your Vue components and templates. ## Testing Vue Components diff --git a/doc/development/new_fe_guide/index.md b/doc/development/new_fe_guide/index.md index 227d03bd86f..152ddcdae64 100644 --- a/doc/development/new_fe_guide/index.md +++ b/doc/development/new_fe_guide/index.md @@ -15,10 +15,6 @@ Learn about all the dependencies that make up our frontend, including some of ou Learn about all the internal JavaScript modules that make up our frontend. -## [Style guides](style/index.md) - -Style guides to keep our code consistent. - ## [Tips](tips.md) Tips from our frontend team to develop more efficiently and effectively. diff --git a/doc/development/new_fe_guide/style/html.md b/doc/development/new_fe_guide/style/html.md index 1445da3f0e1..0b4fce13d90 100644 --- a/doc/development/new_fe_guide/style/html.md +++ b/doc/development/new_fe_guide/style/html.md @@ -1,53 +1,5 @@ -# HTML style guide +--- +redirect_to: '../../fe_guide/style/html.md' +--- -## Buttons - -### Button type - -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> - -// good -<button type="button"></button> -``` - -### Button role - -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/Roles/button_role). - -```html -// bad -<div onClick="doSomething"></div> - -// good -<div role="button" onClick="doSomething"></div> -``` - -## Links - -### Blank target - -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> - -// good -<a href="url" target="_blank" rel="noopener noreferrer"></a> -``` - -### Fake links - -**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> -``` +This document was moved to [another location](../../fe_guide/style/html.md). diff --git a/doc/development/new_fe_guide/style/index.md b/doc/development/new_fe_guide/style/index.md index f073dc56f1f..284862a2be9 100644 --- a/doc/development/new_fe_guide/style/index.md +++ b/doc/development/new_fe_guide/style/index.md @@ -1,15 +1,5 @@ -# Style guides +--- +redirect_to: '../../fe_guide/style/index.md' +--- -## [HTML style guide](html.md) - -## [SCSS style guide](scss.md) - -## [JavaScript style guide](javascript.md) - -## [Vue style guide](vue.md) - -## Tooling - -## [Prettier](prettier.md) - -Our code is automatically formatted with [Prettier](https://prettier.io) to follow our guidelines. +This document was moved to [another location](../../fe_guide/style/index.md). diff --git a/doc/development/new_fe_guide/style/javascript.md b/doc/development/new_fe_guide/style/javascript.md index d31edcb372d..003880c2592 100644 --- a/doc/development/new_fe_guide/style/javascript.md +++ b/doc/development/new_fe_guide/style/javascript.md @@ -1,195 +1,5 @@ -# JavaScript style guide +--- +redirect_to: '../../fe_guide/style/javascript.md' +--- -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. - -> **Tip:** -You can run eslint locally by running `yarn eslint` - -## 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. - -```javascript -// bad -class myClass { - constructor(config) { - this.config = config; - axios.get(this.config.endpoint) - } -} - -// good -class myClass { - constructor(config) { - this.config = config; - } - - makeRequest() { - axios.get(this.config.endpoint) - } -} -const instance = new myClass(); -instance.makeRequest(); -``` - -## 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. - -```javascript -// bad -class myClass { - constructor(config) { - this.config = config; - } - - init() { - document.addEventListener('click', () => {}); - } -} - -// good - -const myFunction = () => { - document.addEventListener('click', () => { - // handle callback here - }); -} -``` - -## Pass element container to constructor - -When your class manipulates the DOM, receive the element container as a parameter. -This is more maintainable and performant. - -```javascript -// bad -class a { - constructor() { - document.querySelector('.b'); - } -} - -// good -class a { - constructor(options) { - options.container.querySelector('.b'); - } -} -``` - -## Use ParseInt - -Use `ParseInt` when converting a numeric string into a number. - -```javascript -// bad -Number('10') - -// good -parseInt('10', 10); -``` - -## CSS Selectors - 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-`. - -```html -// bad -<button class="add-user"></button> - -// good -<button class="js-add-user"></button> -``` - -## Absolute vs relative paths for modules - -Use relative paths if the module you are importing is less than two levels up. - -```javascript -// bad -import GitLabStyleGuide from '~/guides/GitLabStyleGuide'; - -// good -import GitLabStyleGuide from '../GitLabStyleGuide'; -``` - -If the module you are importing is two or more levels up, use an absolute path instead: - -```javascript -// bad -import GitLabStyleGuide from '../../../guides/GitLabStyleGuide'; - -// good -import GitLabStyleGuide from '~/GitLabStyleGuide'; -``` - -Additionally, **do not add to global namespace**. - -## 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. - -## Avoid XSS - -Do not use `innerHTML`, `append()` or `html()` to set content. It opens up too many -vulnerabilities. - -## 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. - -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](https://eslint.org/docs/rules/no-new) -- [class-method-use-this](https://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`. +This document was moved to [another location](../../fe_guide/style/javascript.md). diff --git a/doc/development/new_fe_guide/style/prettier.md b/doc/development/new_fe_guide/style/prettier.md index 17b209d419e..9a95aa96dff 100644 --- a/doc/development/new_fe_guide/style/prettier.md +++ b/doc/development/new_fe_guide/style/prettier.md @@ -1,98 +1,5 @@ -# Formatting with Prettier +--- +redirect_to: '../../fe_guide/tooling.md#formatting-with-prettier' +--- -Our code is automatically formatted with [Prettier](https://prettier.io) to follow our style guides. Prettier is taking care of formatting .js, .vue, and .scss files based on the standard prettier rules. You can find all settings for Prettier in `.prettierrc`. - -## Editor - -The easiest way to include prettier in your workflow is by setting up your preferred editor (all major editors are supported) accordingly. We suggest setting up prettier to run automatically when each file is saved. Find [here](https://prettier.io/docs/en/editors.html) the best way to set it up in your preferred editor. - -Please take care that you only let Prettier format the same file types as the global Yarn script does (.js, .vue, and .scss). In VSCode by example you can easily exclude file formats in your settings file: - -``` - "prettier.disableLanguages": [ - "json", - "markdown" - ], -``` - -## Yarn Script - -The following yarn scripts are available to do global formatting: - -``` -yarn prettier-staged-save -``` - -Updates all currently staged files (based on `git diff`) with Prettier and saves the needed changes. - -``` -yarn prettier-staged -``` - -Checks all currently staged files (based on `git diff`) with Prettier and log which files would need manual updating to the console. - -``` -yarn prettier-all -``` - -Checks all files with Prettier and logs which files need manual updating to the console. - -``` -yarn prettier-all-save -``` - -Formats all files in the repository with Prettier. (This should only be used to test global rule updates otherwise you would end up with huge MR's). - -The source of these Yarn scripts can be found in `/scripts/frontend/prettier.js`. - -### Scripts during Conversion period - -``` -node ./scripts/frontend/prettier.js check-all ./vendor/ -``` - -This will go over all files in a specific folder check it. - -``` -node ./scripts/frontend/prettier.js save-all ./vendor/ -``` - -This will go over all files in a specific folder and save it. - -## VSCode Settings - -### Select Prettier as default formatter - -To select Prettier as a formatter, add the following properties to your User or Workspace Settings: - -```javascript -{ - "[html]": { - "editor.defaultFormatter": "esbenp.prettier-vscode" - }, - "[javascript]": { - "editor.defaultFormatter": "esbenp.prettier-vscode" - }, - "[vue]": { - "editor.defaultFormatter": "esbenp.prettier-vscode" - } -} -``` - -### Format on Save - -To automatically format your files with Prettier, add the following properties to your User or Workspace Settings: - -```javascript -{ - "[html]": { - "editor.formatOnSave": true - }, - "[javascript]": { - "editor.formatOnSave": true - }, - "[vue]": { - "editor.formatOnSave": true - }, -} -``` +This document was moved to [another location](../../fe_guide/tooling.md#formatting-with-prettier). diff --git a/doc/development/new_fe_guide/style/scss.md b/doc/development/new_fe_guide/style/scss.md deleted file mode 100644 index 6f5e818d7db..00000000000 --- a/doc/development/new_fe_guide/style/scss.md +++ /dev/null @@ -1,3 +0,0 @@ -# SCSS style guide - -> TODO: Add content diff --git a/doc/development/new_fe_guide/style/vue.md b/doc/development/new_fe_guide/style/vue.md deleted file mode 100644 index fd9353e0d3f..00000000000 --- a/doc/development/new_fe_guide/style/vue.md +++ /dev/null @@ -1,3 +0,0 @@ -# Vue style guide - -> TODO: Add content |