diff options
author | Filipa Lacerda <filipa@gitlab.com> | 2017-03-22 19:30:54 +0000 |
---|---|---|
committer | Alfredo Sumaran <alfredo@gitlab.com> | 2017-03-22 19:30:54 +0000 |
commit | 8c3bdc853a5237a3bef6e26fcf22132db7e8bd9c (patch) | |
tree | a862ed0847f6dc8d7ffe8dea7141714a51a8387d | |
parent | 3574963bc01097da418b62ae6f87d02359f36f12 (diff) | |
download | gitlab-ce-8c3bdc853a5237a3bef6e26fcf22132db7e8bd9c.tar.gz |
Creates Frontend Style guide
-rw-r--r-- | CONTRIBUTING.md | 8 | ||||
-rw-r--r-- | changelogs/unreleased/28713-fe-style-guide.yml | 4 | ||||
-rw-r--r-- | doc/development/README.md | 2 | ||||
-rw-r--r-- | doc/development/fe_guide/accessibility.md | 13 | ||||
-rw-r--r-- | doc/development/fe_guide/architecture.md | 22 | ||||
-rw-r--r-- | doc/development/fe_guide/design_patterns.md | 78 | ||||
-rw-r--r-- | doc/development/fe_guide/index.md | 92 | ||||
-rw-r--r-- | doc/development/fe_guide/performance.md | 95 | ||||
-rw-r--r-- | doc/development/fe_guide/security.md | 92 | ||||
-rw-r--r-- | doc/development/fe_guide/style_guide_js.md | 408 | ||||
-rw-r--r-- | doc/development/fe_guide/style_guide_scss.md (renamed from doc/development/scss_styleguide.md) | 56 | ||||
-rw-r--r-- | doc/development/fe_guide/testing.md | 129 | ||||
-rw-r--r-- | doc/development/fe_guide/vue.md | 104 | ||||
-rw-r--r-- | doc/development/frontend.md | 511 | ||||
-rw-r--r-- | doc/development/rake_tasks.md | 14 | ||||
-rw-r--r-- | doc/development/testing.md | 11 |
16 files changed, 1103 insertions, 536 deletions
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a285e8ab74f..275c0cd1777 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -82,7 +82,7 @@ If a contributor is no longer actively working on a submitted merge request we can decide that the merge request will be finished by one of our [Merge request coaches][team] or close the merge request. We make this decision based on how important the change is for our product vision. If a Merge request -coach is going to finish the merge request we assign the +coach is going to finish the merge request we assign the ~"coach will finish" label. ## Helping others @@ -479,8 +479,7 @@ merge request: 1. [Rails](https://github.com/bbatsov/rails-style-guide) 1. [Newlines styleguide][newlines-styleguide] 1. [Testing](doc/development/testing.md) -1. [JavaScript (ES6)](https://github.com/airbnb/javascript) -1. [JavaScript (ES5)](https://github.com/airbnb/javascript/tree/es5-deprecated/es5) +1. [JavaScript styleguide][js-styleguide] 1. [SCSS styleguide][scss-styleguide] 1. [Shell commands](doc/development/shell_commands.md) created by GitLab contributors to enhance security @@ -549,7 +548,8 @@ available at [http://contributor-covenant.org/version/1/1/0/](http://contributor [rss-naming]: https://github.com/bbatsov/ruby-style-guide/blob/master/README.md#naming [changelog]: doc/development/changelog.md "Generate a changelog entry" [doc-styleguide]: doc/development/doc_styleguide.md "Documentation styleguide" -[scss-styleguide]: doc/development/scss_styleguide.md "SCSS styleguide" +[js-styleguide]: doc/development/fe_guide/style_guide_js.md "JavaScript styleguide" +[scss-styleguide]: doc/development/fe_guide/style_guide_scss.md "SCSS styleguide" [newlines-styleguide]: doc/development/newlines_styleguide.md "Newlines styleguide" [UX Guide for GitLab]: http://docs.gitlab.com/ce/development/ux_guide/ [license-finder-doc]: doc/development/licensing.md diff --git a/changelogs/unreleased/28713-fe-style-guide.yml b/changelogs/unreleased/28713-fe-style-guide.yml new file mode 100644 index 00000000000..57edb43e27f --- /dev/null +++ b/changelogs/unreleased/28713-fe-style-guide.yml @@ -0,0 +1,4 @@ +--- +title: Adds Frontend Styleguide to documentation +merge_request: 9961 +author: diff --git a/doc/development/README.md b/doc/development/README.md index 265df98fb87..e27e7fc7d19 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -15,7 +15,7 @@ - [SQL Migration Style Guide](migration_style_guide.md) for creating safe SQL migrations - [Testing standards and style guidelines](testing.md) - [UX guide](ux_guide/index.md) for building GitLab with existing CSS styles and elements -- [Frontend guidelines](frontend.md) +- [Frontend guidelines](fe_guide/index.md) - [SQL guidelines](sql.md) for working with SQL queries - [Sidekiq guidelines](sidekiq_style_guide.md) for working with Sidekiq workers - [`Gemfile` guidelines](gemfile.md) diff --git a/doc/development/fe_guide/accessibility.md b/doc/development/fe_guide/accessibility.md new file mode 100644 index 00000000000..366b220cbb2 --- /dev/null +++ b/doc/development/fe_guide/accessibility.md @@ -0,0 +1,13 @@ +# Accessibility + +## Resources + +[Chrome Accessibility Developer Tools][chrome-accessibility-developer-tools] +are useful for testing for potential accessibility problems in GitLab. + +Accessibility best-practices and more in-depth information is available on +[the Audit Rules page][audit-rules] for the Chrome Accessibility Developer Tools. + + +[chrome-accessibility-developer-tools]: https://github.com/GoogleChrome/accessibility-developer-tools +[audit-rules]: https://github.com/GoogleChrome/accessibility-developer-tools/wiki/Audit-Rules diff --git a/doc/development/fe_guide/architecture.md b/doc/development/fe_guide/architecture.md new file mode 100644 index 00000000000..aebb22caa15 --- /dev/null +++ b/doc/development/fe_guide/architecture.md @@ -0,0 +1,22 @@ +# Architecture + +When you are developing a new feature that requires architectural design, or if +you are changing the fundamental design of an existing feature, make sure it is +discussed with one of the Frontend Architecture Experts. + +A Frontend Architect is an expert who makes high-level Frontend design decisions +and decides on technical standards, including coding standards and frameworks. + +Architectural decisions should be accessible to everyone, so please document +them in the relevant Merge Request discussion or by updating our documentation +when appropriate. + +You can find the Frontend Architecture experts on the [team page][team-page]. + +## Examples + +You can find documentation about the desired architecture for a new feature +built with Vue.js [here][vue-section]. + +[team-page]: https://about.gitlab.com/team +[vue-section]: vue.md#frontend.html#how-to-build-a-new-feature-with-vue-js diff --git a/doc/development/fe_guide/design_patterns.md b/doc/development/fe_guide/design_patterns.md new file mode 100644 index 00000000000..e05887a19af --- /dev/null +++ b/doc/development/fe_guide/design_patterns.md @@ -0,0 +1,78 @@ +# Design Patterns + +## Singletons + +When exactly one object is needed for a given task, prefer to define it as a +`class` rather than as an object literal. Prefer also to explicitly restrict +instantiation, unless flexibility is important (e.g. for testing). + +```javascript +// bad + +const MyThing = { + prop1: 'hello', + method1: () => {} +}; + +export default MyThing; + +// good + +class MyThing { + constructor() { + this.prop1 = 'hello'; + } + method1() {} +} + +export default new MyThing(); + +// best + +export default class MyThing { + constructor() { + if (!this.prototype.singleton) { + this.init(); + this.prototype.singleton = this; + } + return this.prototype.singleton; + } + + init() { + this.prop1 = 'hello'; + } + + method1() {} +} + +``` + +## Manipulating the DOM in a JS Class + +When writing a class that needs to manipulate the DOM guarantee a container option is provided. +This is useful when we need that class to be instantiated more than once in the same page. + +Bad: +```javascript +class Foo { + constructor() { + document.querySelector('.bar'); + } +} +new Foo(); +``` + +Good: +```javascript +class Foo { + constructor(opts) { + document.querySelector(`${opts.container} .bar`); + } +} + +new Foo({ container: '.my-element' }); +``` +You can find an example of the above in this [class][container-class-example]; + + +[container-class-example]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/assets/javascripts/mini_pipeline_graph_dropdown.js diff --git a/doc/development/fe_guide/index.md b/doc/development/fe_guide/index.md new file mode 100644 index 00000000000..f963bffde37 --- /dev/null +++ b/doc/development/fe_guide/index.md @@ -0,0 +1,92 @@ +# Frontend Development Guidelines + +This document describes various guidelines to ensure consistency and quality +across GitLab's frontend team. + +## Overview + +GitLab is built on top of [Ruby on Rails][rails] using [Haml][haml] with +[Hamlit][hamlit]. Be wary of [the limitations that come with using +Hamlit][hamlit-limits]. We also use [SCSS][scss] and plain JavaScript with +modern ECMAScript standards supported through [Babel][babel] and ES module +support through [webpack][webpack]. + +We also utilize [webpack][webpack] to handle the bundling, minification, and +compression of our assets. + +Working with our frontend assets requires Node (v4.3 or greater) and Yarn +(v0.17 or greater). You can find information on how to install these on our +[installation guide][install]. + +[jQuery][jquery] is used throughout the application's JavaScript, with +[Vue.js][vue] for particularly advanced, dynamic elements. + +### Browser Support + +For our currently-supported browsers, see our [requirements][requirements]. + +--- + +## [Architecture](architecture.md) +How we go about making fundamental design decisions in GitLab's frontend team +or make changes to our frontend development guidelines. + +--- + +## [Testing](testing.md) +How we write frontend tests, run the GitLab test suite, and debug test related +issues. + +--- + +## [Design Patterns](design_patterns.md) +Common JavaScript design patterns in GitLab's codebase. + +--- + +## [Vue.js Best Practices](vue.md) +Vue specific design patterns and practices. + +--- + +## Style Guides + +### [JavaScript Style Guide](style_guide_js.md) + +We use eslint to enforce our JavaScript style guides. Our guide is based on +the excellent [Airbnb][airbnb-js-style-guide] style guide with a few small +changes. + +### [SCSS Style Guide](style_guide_scss.md) + +Our SCSS conventions which are enforced through [scss-lint][scss-lint]. + +--- + +## [Performance](performance.md) +Best practices for monitoring and maximizing frontend performance. + +--- + +## [Security](security.md) +Frontend security practices. + +--- + +## [Accessibility](accessibility.md) +Our accessibility standards and resources. + + +[rails]: http://rubyonrails.org/ +[haml]: http://haml.info/ +[hamlit]: https://github.com/k0kubun/hamlit +[hamlit-limits]: https://github.com/k0kubun/hamlit/blob/master/REFERENCE.md#limitations +[scss]: http://sass-lang.com/ +[babel]: https://babeljs.io/ +[webpack]: https://webpack.js.org/ +[jquery]: https://jquery.com/ +[vue]: http://vuejs.org/ +[airbnb-js-style-guide]: https://github.com/airbnb/javascript +[scss-lint]: https://github.com/brigade/scss-lint +[install]: ../../install/installation.md#4-node +[requirements]: ../../install/requirements.md#supported-web-browsers diff --git a/doc/development/fe_guide/performance.md b/doc/development/fe_guide/performance.md new file mode 100644 index 00000000000..2d76bb18cff --- /dev/null +++ b/doc/development/fe_guide/performance.md @@ -0,0 +1,95 @@ +# Performance + +## Best Practices + +### Realtime Components + +When writing code for realtime features we have to keep a couple of things in mind: +1. Do not overload the server with requests. +1. It should feel realtime. + +Thus, we must strike a balance between sending requests and the feeling of realtime. +Use the following rules when creating realtime solutions. + +1. The server will tell you how much to poll by sending `Poll-Interval` in the header. +Use that as your polling interval. This way it is easy for system administrators to change the +polling rate. +A `Poll-Interval: -1` means you should disable polling, and this must be implemented. +1. A response with HTTP status `4XX` or `5XX` should disable polling as well. +1. Use a common library for polling. +1. Poll on active tabs only. Use a common library to find out which tab currently has eyes on it. +Please use [Focus](https://gitlab.com/andrewn/focus). Specifically [Eyeballs Detector](https://gitlab.com/andrewn/focus/blob/master/lib/eyeballs-detector.js). +1. Use regular polling intervals, do not use backoff polling, or jitter, as the interval will be +controlled by the server. +1. The backend code will most likely be using etags. You do not and should not check for status +`304 Not Modified`. The browser will transform it for you. + +## Reducing Asset Footprint + +### Page-specific JavaScript + +Certain pages may require the use of a third party library, such as [d3][d3] for +the User Activity Calendar and [Chart.js][chartjs] for the Graphs pages. These +libraries increase the page size significantly, and impact load times due to +bandwidth bottlenecks and the browser needing to parse more JavaScript. + +In cases where libraries are only used on a few specific pages, we use +"page-specific JavaScript" to prevent the main `main.js` file from +becoming unnecessarily large. + +Steps to split page-specific JavaScript from the main `main.js`: + +1. Create a directory for the specific page(s), e.g. `graphs/`. +1. In that directory, create a `namespace_bundle.js` file, e.g. `graphs_bundle.js`. +1. Add the new "bundle" file to the list of entry files in `config/webpack.config.js`. + - For example: `graphs: './graphs/graphs_bundle.js',`. +1. Move code reliant on these libraries into the `graphs` directory. +1. In `graphs_bundle.js` add CommonJS `require('./path_to_some_component.js');` statements to load any other files in this directory. Make sure to use relative urls. +1. In the relevant views, add the scripts to the page with the following: + +```haml +- content_for :page_specific_javascripts do + = page_specific_javascript_bundle_tag('lib_chart') + = page_specific_javascript_bundle_tag('graphs') +``` + +The above loads `chart.js` and `graphs_bundle.js` for this page only. `chart.js` +is separated from the bundle file so it can be cached separately from the bundle +and reused for other pages that also rely on the library. For an example, see +[this Haml file][page-specific-js-example]. + +### Code Splitting + +> *TODO* flesh out this section once webpack is ready for code-splitting + +### Minimizing page size + +A smaller page size means the page loads faster (especially important on mobile +and poor connections), the page is parsed more quickly by the browser, and less +data is used for users with capped data plans. + +General tips: + +- Don't add new fonts. +- Prefer font formats with better compression, e.g. WOFF2 is better than WOFF, which is better than TTF. +- Compress and minify assets wherever possible (For CSS/JS, Sprockets and webpack do this for us). +- If some functionality can reasonably be achieved without adding extra libraries, avoid them. +- Use page-specific JavaScript as described above to dynamically load libraries that are only needed on certain pages. + +------- + +## Additional Resources + +- [WebPage Test][web-page-test] for testing site loading time and size. +- [Google PageSpeed Insights][pagespeed-insights] grades web pages and provides feedback to improve the page. +- [Profiling with Chrome DevTools][google-devtools-profiling] +- [Browser Diet][browser-diet] is a community-built guide that catalogues practical tips for improving web page performance. + + +[web-page-test]: http://www.webpagetest.org/ +[pagespeed-insights]: https://developers.google.com/speed/pagespeed/insights/ +[google-devtools-profiling]: https://developers.google.com/web/tools/chrome-devtools/profile/?hl=en +[browser-diet]: https://browserdiet.com/ +[d3]: https://d3js.org/ +[chartjs]: http://www.chartjs.org/ +[page-specific-js-example]: https://gitlab.com/gitlab-org/gitlab-ce/blob/13bb9ed77f405c5f6ee4fdbc964ecf635c9a223f/app/views/projects/graphs/_head.html.haml#L6-8 diff --git a/doc/development/fe_guide/security.md b/doc/development/fe_guide/security.md new file mode 100644 index 00000000000..19e72c1d368 --- /dev/null +++ b/doc/development/fe_guide/security.md @@ -0,0 +1,92 @@ +# Security +### Resources + +[Mozilla’s HTTP Observatory CLI][observatory-cli] and the +[Qualys SSL Labs Server Test][qualys-ssl] are good resources for finding +potential problems and ensuring compliance with security best practices. + +<!-- Uncomment these sections when CSP/SRI are implemented. +### Content Security Policy (CSP) + +Content Security Policy is a web standard that intends to mitigate certain +forms of Cross-Site Scripting (XSS) as well as data injection. + +Content Security Policy rules should be taken into consideration when +implementing new features, especially those that may rely on connection with +external services. + +GitLab's CSP is used for the following: + +- Blocking plugins like Flash and Silverlight from running at all on our pages. +- Blocking the use of scripts and stylesheets downloaded from external sources. +- Upgrading `http` requests to `https` when possible. +- Preventing `iframe` elements from loading in most contexts. + +Some exceptions include: + +- Scripts from Google Analytics and Piwik if either is enabled. +- Connecting with GitHub, Bitbucket, GitLab.com, etc. to allow project importing. +- Connecting with Google, Twitter, GitHub, etc. to allow OAuth authentication. + +We use [the Secure Headers gem][secure_headers] to enable Content +Security Policy headers in the GitLab Rails app. + +Some resources on implementing Content Security Policy: + +- [MDN Article on CSP][mdn-csp] +- [GitHub’s CSP Journey on the GitHub Engineering Blog][github-eng-csp] +- The Dropbox Engineering Blog's series on CSP: [1][dropbox-csp-1], [2][dropbox-csp-2], [3][dropbox-csp-3], [4][dropbox-csp-4] + +### Subresource Integrity (SRI) + +Subresource Integrity prevents malicious assets from being provided by a CDN by +guaranteeing that the asset downloaded is identical to the asset the server +is expecting. + +The Rails app generates a unique hash of the asset, which is used as the +asset's `integrity` attribute. The browser generates the hash of the asset +on-load and will reject the asset if the hashes do not match. + +All CSS and JavaScript assets should use Subresource Integrity. + +Some resources on implementing Subresource Integrity: + +- [MDN Article on SRI][mdn-sri] +- [Subresource Integrity on the GitHub Engineering Blog][github-eng-sri] + +--> + +### Including external resources + +External fonts, CSS, and JavaScript should never be used with the exception of +Google Analytics and Piwik - and only when the instance has enabled it. Assets +should always be hosted and served locally from the GitLab instance. Embedded +resources via `iframes` should never be used except in certain circumstances +such as with ReCaptcha, which cannot be used without an `iframe`. + +### Avoiding inline scripts and styles + +In order to protect users from [XSS vulnerabilities][xss], we will disable +inline scripts in the future using Content Security Policy. + +While inline scripts can be useful, they're also a security concern. If +user-supplied content is unintentionally left un-sanitized, malicious users can +inject scripts into the web app. + +Inline styles should be avoided in almost all cases, they should only be used +when no alternatives can be found. This allows reusability of styles as well as +readability. + + +[observatory-cli]: https://github.com/mozilla/http-observatory-cli +[qualys-ssl]: https://www.ssllabs.com/ssltest/analyze.html +[secure_headers]: https://github.com/twitter/secureheaders +[mdn-csp]: https://developer.mozilla.org/en-US/docs/Web/Security/CSP +[github-eng-csp]: http://githubengineering.com/githubs-csp-journey/ +[dropbox-csp-1]: https://blogs.dropbox.com/tech/2015/09/on-csp-reporting-and-filtering/ +[dropbox-csp-2]: https://blogs.dropbox.com/tech/2015/09/unsafe-inline-and-nonce-deployment/ +[dropbox-csp-3]: https://blogs.dropbox.com/tech/2015/09/csp-the-unexpected-eval/ +[dropbox-csp-4]: https://blogs.dropbox.com/tech/2015/09/csp-third-party-integrations-and-privilege-separation/ +[mdn-sri]: https://developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integrity +[github-eng-sri]: http://githubengineering.com/subresource-integrity/ +[xss]: https://en.wikipedia.org/wiki/Cross-site_scripting diff --git a/doc/development/fe_guide/style_guide_js.md b/doc/development/fe_guide/style_guide_js.md new file mode 100644 index 00000000000..034cfe73d33 --- /dev/null +++ b/doc/development/fe_guide/style_guide_js.md @@ -0,0 +1,408 @@ +# Style guides and linting +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][eslintrc] for specific rules and patterns. + +### Common + +#### 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 + + ```javascript + // bad + /* eslint-disable */ + + // better + /* eslint-disable some-rule, some-other-rule */ + + // best + // nothing :) + ``` + +- 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(); + ``` + +- 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'; + ``` + +- **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 */ + ``` + +#### Modules, Imports, and Exports +- Use ES module syntax to import modules + + ```javascript + // bad + require('foo'); + + // good + import Foo from 'foo'; + + // bad + module.exports = Foo; + + // good + export default Foo; + ``` + +- Relative paths + + Unless you are writing a test, always reference other scripts using relative paths instead of `~` + + In **app/assets/javascripts**: + ```javascript + // bad + import Foo from '~/foo' + + // good + import Foo from '../foo'; + ``` + + In **spec/javascripts**: + ```javascript + // bad + import Foo from '../../app/assets/javascripts/foo' + + // good + import Foo from '~/foo'; + ``` + +- Avoid using IIFE. Although we have a lot of examples of files which wrap their contents in IIFEs (immediately-invoked function expressions), this is no longer necessary after the transition from Sprockets to webpack. Do not use them anymore and feel free to remove them when refactoring legacy code. + +- Avoid adding to the global namespace. + + ```javascript + // bad + window.MyClass = class { /* ... */ }; + + // good + export default class MyClass { /* ... */ } + ``` + +- Side effects are forbidden in any script which contains exports + + ```javascript + // bad + export default class 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. + + ```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); + ``` + +- Avoid constructors with side-effects + + +#### Parse Strings into Numbers +- `parseInt()` is preferable over `Number()` or `+` + + ```javascript + // bad + +'10' // 10 + + // good + Number('10') // 10 + + // better + parseInt('10', 10); + ``` + + +### Vue.js + + +#### Basic Rules +- Only include one Vue.js component per file. +- Export components as plain objects: + + ```javascript + export default { + template: `<h1>I'm a component</h1> + } + ``` + +#### Naming +- **Extensions**: Use `.vue` extension for Vue components. +- **Reference Naming**: Use PascalCase for Vue components and camelCase for their instances: + + ```javascript + // bad + import cardBoard from 'cardBoard'; + + // good + import CardBoard from 'cardBoard' + + // bad + components: { + CardBoard: CardBoard + }; + + // good + components: { + cardBoard: CardBoard + }; + ``` +- **Props Naming**: Avoid using DOM component prop names. + + ```javascript + // bad + <component class="btn"> + + // good + <component cssClass="btn"> + ``` + +#### Alignment +- Follow these alignment styles for the template method: + + ```javascript + // bad + <component v-if="bar" + param="baz" /> + + // good + <component + v-if="bar" + param="baz" + /> + + // if props fit in one line then keep it on the same line + <component bar="bar" /> + ``` + +#### Quotes +- 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 +- Props should be declared as an object + + ```javascript + // bad + props: ['foo'] + + // good + props: { + foo: { + type: String, + required: false, + default: 'bar' + } + } + ``` + +- 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' + } + } + ``` + +- Default key should always be provided if the prop is not required: + + ```javascript + // bad + props: { + foo: { + type: String, + required: false, + } + } + + // good + props: { + foo: { + type: String, + required: false, + default: 'bar' + } + } + + // good + props: { + foo: { + type: String, + required: true + } + } + ``` + +#### Data +- `data` method should always be a function + + ```javascript + // bad + data: { + foo: 'foo' + } + + // good + data() { + return { + foo: 'foo' + }; + } + ``` + +#### Directives + +- Shorthand `@` is preferable over `v-on` + + ```javascript + // bad + <component v-on:click="eventHandler"/> + + + // good + <component @click="eventHandler"/> + ``` + +- Shorthand `:` is preferable over `v-bind` + + ```javascript + // bad + <component v-bind:class="btn"/> + + + // good + <component :class="btn"/> + ``` + +#### Closing tags +- Prefer self closing component tags + + ```javascript + // bad + <component></component> + + // good + <component /> + ``` + +#### Ordering +- 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` + + +## SCSS +- [SCSS](style_guide_scss.md) + +[airbnb-js-style-guide]: https://github.com/airbnb/javascript +[eslintrc]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/.eslintrc diff --git a/doc/development/scss_styleguide.md b/doc/development/fe_guide/style_guide_scss.md index a79f4073cde..77b308c4a43 100644 --- a/doc/development/scss_styleguide.md +++ b/doc/development/fe_guide/style_guide_scss.md @@ -35,7 +35,7 @@ between the property and its value. ```scss // Bad -.container-item { +.container-item { width: 100px; height: 100px; margin-top: 0; } @@ -63,7 +63,7 @@ between the property and its value. } ``` -Note that there is an exception for single-line rulesets, although these are +Note that there is an exception for single-line rulesets, although these are not typically recommended. ```scss @@ -72,7 +72,7 @@ p { margin: 0; padding: 0; } ### Colors -HEX (hexadecimal) colors should use shorthand where possible, and should use +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`. @@ -111,7 +111,7 @@ p { ### Semicolons -Always include semicolons after every property. When the stylesheets are +Always include semicolons after every property. When the stylesheets are minified, the semicolons will be removed automatically. ```scss @@ -144,7 +144,7 @@ padding: 10px; ### Zero Units -Omit length units on zero values, they're unnecessary and not including them +Omit length units on zero values, they're unnecessary and not including them is slightly more performant. ```scss @@ -161,36 +161,56 @@ is slightly more performant. ### 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 +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: +1. There isn't already one +2. There isn't a similar one we can use instead. + ## Linting -We use [SCSS Lint][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 +We use [SCSS Lint][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 +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 +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]. ### Fixing issues -If you want to automate changing a large portion of the codebase to conform to +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 +[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 +If you want a line or set of lines to be ignored by the linter, you can use `// scss-lint:disable RuleName` ([more info][disabling-linters]): ```scss @@ -203,8 +223,8 @@ If you want a line or set of lines to be ignored by the linter, you can use ``` 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 +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 diff --git a/doc/development/fe_guide/testing.md b/doc/development/fe_guide/testing.md new file mode 100644 index 00000000000..bb6adeacc4c --- /dev/null +++ b/doc/development/fe_guide/testing.md @@ -0,0 +1,129 @@ +# Frontend Testing + +There are two types of tests you'll encounter while developing frontend code +at GitLab. We use Karma and Jasmine for JavaScript unit testing, and RSpec +feature tests with Capybara for integration testing. + +Feature tests need to be written for all new features. Regression tests ought +to be written for all bug fixes to prevent them from recurring in the future. + +See [the Testing Standards and Style Guidelines](/doc/development/testing.md) +for more information on general testing practices at GitLab. + +## Karma test suite + +GitLab uses the [Karma][karma] test runner with [Jasmine][jasmine] as its test +framework for our JavaScript unit tests. For tests that rely on DOM +manipulation we use fixtures which are pre-compiled from HAML source files and +served during testing by the [jasmine-jquery][jasmine-jquery] plugin. + +### Running frontend tests + +`rake karma` runs the frontend-only (JavaScript) tests. +It consists of two subtasks: + +- `rake karma:fixtures` (re-)generates fixtures +- `rake karma:tests` actually executes the tests + +As long as the fixtures don't change, `rake karma:tests` (or `yarn karma`) +is sufficient (and saves you some time). + +### Live testing and focused testing + +While developing locally, it may be helpful to keep karma running so that you +can get instant feedback on as you write tests and modify code. To do this +you can start karma with `npm run karma-start`. It will compile the javascript +assets and run a server at `http://localhost:9876/` where it will automatically +run the tests on any browser which connects to it. You can enter that url on +multiple browsers at once to have it run the tests on each in parallel. + +While karma is running, any changes you make will instantly trigger a recompile +and retest of the entire test suite, so you can see instantly if you've broken +a test with your changes. You can use [jasmine focused][jasmine-focus] or +excluded tests (with `fdescribe` or `xdescribe`) to get karma to run only the +tests you want while you're working on a specific feature, but make sure to +remove these directives when you commit your code. + +## RSpec Feature Integration Tests + +Information on setting up and running RSpec integration tests with +[Capybara][capybara] can be found in the +[general testing guide](/doc/development/testing.md). + +## Gotchas + +### Errors due to use of unsupported JavaScript features + +Similar errors will be thrown if you're using JavaScript features not yet +supported by the PhantomJS test runner which is used for both Karma and RSpec +tests. We polyfill some JavaScript objects for older browsers, but some +features are still unavailable: + +- Array.from +- Array.first +- Async functions +- Generators +- Array destructuring +- For..Of +- Symbol/Symbol.iterator +- Spread + +Until these are polyfilled appropriately, they should not be used. Please +update this list with additional unsupported features. + +### RSpec errors due to JavaScript + +By default RSpec unit tests will not run JavaScript in the headless browser +and will simply rely on inspecting the HTML generated by rails. + +If an integration test depends on JavaScript to run correctly, you need to make +sure the spec is configured to enable JavaScript when the tests are run. If you +don't do this you'll see vague error messages from the spec runner. + +To enable a JavaScript driver in an `rspec` test, add `js: true` to the +individual spec or the context block containing multiple specs that need +JavaScript enabled: + +```ruby + +# For one spec +it 'presents information about abuse report', js: true do + # assertions... +end + +describe "Admin::AbuseReports", js: true do + it 'presents information about abuse report' do + # assertions... + end + it 'shows buttons for adding to abuse report' do + # assertions... + end +end +``` + +### Spinach errors due to missing JavaScript + +> **Note:** Since we are discouraging the use of Spinach when writing new +> feature tests, you shouldn't ever need to use this. This information is kept +> available for legacy purposes only. + +In Spinach, the JavaScript driver is enabled differently. In the `*.feature` +file for the failing spec, add the `@javascript` flag above the Scenario: + +``` +@javascript +Scenario: Developer can approve merge request + Given I am a "Shop" developer + And I visit project "Shop" merge requests page + And merge request 'Bug NS-04' must be approved + And I click link "Bug NS-04" + When I click link "Approve" + Then I should see approved merge request "Bug NS-04" + +``` + +[capybara]: http://teamcapybara.github.io/capybara/ +[jasmine]: https://jasmine.github.io/ +[jasmine-focus]: https://jasmine.github.io/2.5/focused_specs.html +[jasmine-jquery]: https://github.com/velesin/jasmine-jquery +[karma]: http://karma-runner.github.io/ diff --git a/doc/development/fe_guide/vue.md b/doc/development/fe_guide/vue.md new file mode 100644 index 00000000000..3e3406e7d6a --- /dev/null +++ b/doc/development/fe_guide/vue.md @@ -0,0 +1,104 @@ +# Vue + +For more complex frontend features, we recommend using Vue.js. It shares +some ideas with React.js as well as Angular. + +To get started with Vue, read through [their documentation][vue-docs]. + +## When to use Vue.js + +We recommend using Vue for more complex features. Here are some guidelines for when to use Vue.js: + +- If you are starting a new feature or refactoring an old one that highly interacts with the DOM; +- For real time data updates; +- If you are creating a component that will be reused elsewhere; + +## When not to use Vue.js + +We don't want to refactor all GitLab frontend code into Vue.js, here are some guidelines for +when not to use Vue.js: + +- Adding or changing static information; +- Features that highly depend on jQuery will be hard to work with Vue.js + +As always, the Frontend Architectural Experts are available to help with any Vue or JavaScript questions. + +## How to build a new feature with Vue.js + +**Components, Stores and Services** + +In some features implemented with Vue.js, like the [issue board][issue-boards] +or [environments table][environments-table] +you can find a clear separation of concerns: + +``` +new_feature +├── components +│ └── component.js.es6 +│ └── ... +├── store +│ └── new_feature_store.js.es6 +├── service +│ └── new_feature_service.js.es6 +├── new_feature_bundle.js.es6 +``` +_For consistency purposes, we recommend you to follow the same structure._ + +Let's look into each of them: + +**A `*_bundle.js` file** + +This is the index file of your new feature. This is where the root Vue instance +of the new feature should be. + +The Store and the Service should be imported and initialized in this file and provided as a prop to the main component. + +Don't forget to follow [these steps.][page_specific_javascript] + +**A folder for Components** + +This folder holds all components that are specific of this new feature. +If you need to use or create a component that will probably be used somewhere +else, please refer to `vue_shared/components`. + +A good thumb rule to know when you should create a component is to think if +it will be reusable elsewhere. + +For example, tables are used in a quite amount of places across GitLab, a table +would be a good fit for a component. On the other hand, a table cell used only +in one table would not be a good use of this pattern. + +You can read more about components in Vue.js site, [Component System][component-system] + +**A folder for the Store** + +The Store is a class that allows us to manage the state in a single +source of truth. + +The concept we are trying to follow is better explained by Vue documentation +itself, please read this guide: [State Management][state-management] + +**A folder for the Service** + +The Service is used only to communicate with the server. +It does not store or manipulate any data. +We use [vue-resource][vue-resource-repo] to +communicate with the server. + +The [issue boards service][issue-boards-service] +is a good example of this pattern. + +## Style guide + +Please refer to the Vue section of our [style guide](style_guide_js.md#vuejs) +for best practices while writing your Vue components and templates. + + +[vue-docs]: http://vuejs.org/guide/index.html +[issue-boards]: https://gitlab.com/gitlab-org/gitlab-ce/tree/master/app/assets/javascripts/boards +[environments-table]: https://gitlab.com/gitlab-org/gitlab-ce/tree/master/app/assets/javascripts/environments +[page_specific_javascript]: https://docs.gitlab.com/ce/development/frontend.html#page-specific-javascript +[component-system]: https://vuejs.org/v2/guide/#Composing-with-Components +[state-management]: https://vuejs.org/v2/guide/state-management.html#Simple-State-Management-from-Scratch +[vue-resource-repo]: https://github.com/pagekit/vue-resource +[issue-boards-service]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/assets/javascripts/boards/services/board_service.js.es6 diff --git a/doc/development/frontend.md b/doc/development/frontend.md index 50105a486d0..f46cc377f95 100644 --- a/doc/development/frontend.md +++ b/doc/development/frontend.md @@ -1,511 +1,4 @@ -# Frontend Development Guidelines - -This document describes various guidelines to ensure consistency and quality -across GitLab's frontend team. - -## Overview - -GitLab is built on top of [Ruby on Rails][rails] using [Haml][haml] with -[Hamlit][hamlit]. Be wary of [the limitations that come with using -Hamlit][hamlit-limits]. We also use [SCSS][scss] and plain JavaScript with -[ES6 by way of Babel][es6]. - -The asset pipeline is [Sprockets][sprockets], which handles the concatenation, -minification, and compression of our assets. - -[jQuery][jquery] is used throughout the application's JavaScript, with -[Vue.js][vue] for particularly advanced, dynamic elements. - -### Architecture - -The Frontend Architect is an expert who makes high-level frontend design choices -and decides on technical standards, including coding standards, and frameworks. - -When you are assigned a new feature that requires architectural design, -make sure it is discussed with one of the Frontend Architecture Experts. - -This rule also applies if you plan to change the architecture of an existing feature. - -These decisions should be accessible to everyone, so please document it on the Merge Request. - -You can find the Frontend Architecture experts on the [team page][team-page]. - -You can find documentation about the desired architecture for a new feature built with Vue.js in [here][vue-section]. - -### Realtime - -When writing code for realtime features we have to keep a couple of things in mind: -1. Do not overload the server with requests. -1. It should feel realtime. - -Thus, we must strike a balance between sending requests and the feeling of realtime. -Use the following rules when creating realtime solutions. - -1. The server will tell you how much to poll by sending `Poll-Interval` in the header. -Use that as your polling interval. This way it is easy for system administrators to change the -polling rate. -A `Poll-Interval: -1` means you should disable polling, and this must be implemented. -1. A response with HTTP status `4XX` or `5XX` should disable polling as well. -1. Use a common library for polling. -1. Poll on active tabs only. Use a common library to find out which tab currently has eyes on it. -Please use [Focus](https://gitlab.com/andrewn/focus). Specifically [Eyeballs Detector](https://gitlab.com/andrewn/focus/blob/master/lib/eyeballs-detector.js). -1. Use regular polling intervals, do not use backoff polling, or jitter, as the interval will be -controlled by the server. -1. The backend code will most likely be using etags. You do not and should not check for status -`304 Not Modified`. The browser will transform it for you. - -### Vue - -For more complex frontend features, we recommend using Vue.js. It shares -some ideas with React.js as well as Angular. - -To get started with Vue, read through [their documentation][vue-docs]. - -#### How to build a new feature with Vue.js -**Components, Stores and Services** - -In some features implemented with Vue.js, like the [issue board][issue-boards] -or [environments table][environments-table] -you can find a clear separation of concerns: - -``` -new_feature -├── components -│ └── component.js.es6 -│ └── ... -├── store -│ └── new_feature_store.js.es6 -├── service -│ └── new_feature_service.js.es6 -├── new_feature_bundle.js.es6 -``` -_For consistency purposes, we recommend you to follow the same structure._ - -Let's look into each of them: - -**A `*_bundle.js` file** - -This is the index file of your new feature. This is where the root Vue instance -of the new feature should be. - -The Store and the Service should be imported and initialized in this file and provided as a prop to the main component. - -Don't forget to follow [these steps.][page_specific_javascript] - -**A folder for Components** - -This folder holds all components that are specific of this new feature. -If you need to use or create a component that will probably be used somewhere -else, please refer to `vue_shared/components`. - -A good thumb rule to know when you should create a component is to think if -it will be reusable elsewhere. - -For example, tables are used in a quite amount of places across GitLab, a table -would be a good fit for a component. -On the other hand, a table cell used only in on table, would not be a good use -of this pattern. - -You can read more about components in Vue.js site, [Component System][component-system] - -**A folder for the Store** - -The Store is a class that allows us to manage the state in a single -source of truth. - -The concept we are trying to follow is better explained by Vue documentation -itself, please read this guide: [State Management][state-management] - -**A folder for the Service** - -The Service is used only to communicate with the server. -It does not store or manipulate any data. -We use [vue-resource][vue-resource-repo] to -communicate with the server. - -The [issue boards service][issue-boards-service] -is a good example of this pattern. - -## Performance - -### Resources - -- [WebPage Test][web-page-test] for testing site loading time and size. -- [Google PageSpeed Insights][pagespeed-insights] grades web pages and provides feedback to improve the page. -- [Profiling with Chrome DevTools][google-devtools-profiling] -- [Browser Diet][browser-diet] is a community-built guide that catalogues practical tips for improving web page performance. - -### Page-specific JavaScript - -Certain pages may require the use of a third party library, such as [d3][d3] for -the User Activity Calendar and [Chart.js][chartjs] for the Graphs pages. These -libraries increase the page size significantly, and impact load times due to -bandwidth bottlenecks and the browser needing to parse more JavaScript. - -In cases where libraries are only used on a few specific pages, we use -"page-specific JavaScript" to prevent the main `application.js` file from -becoming unnecessarily large. - -Steps to split page-specific JavaScript from the main `application.js`: - -1. Create a directory for the specific page(s), e.g. `graphs/`. -1. In that directory, create a `namespace_bundle.js` file, e.g. `graphs_bundle.js`. -1. In `graphs_bundle.js` add the line `//= require_tree .`, this adds all other files in the directory to the bundle. -1. Add any necessary libraries to `app/assets/javascripts/lib/`, all files directly descendant from this directory will be precompiled as separate assets, in this case `chart.js` would be added. -1. Add the new "bundle" file to the list of precompiled assets in -`config/application.rb`. - - For example: `config.assets.precompile << "graphs/graphs_bundle.js"`. -1. Move code reliant on these libraries into the `graphs` directory. -1. In the relevant views, add the scripts to the page with the following: - -```haml -- content_for :page_specific_javascripts do - = page_specific_javascript_tag('lib/chart.js') - = page_specific_javascript_tag('graphs/graphs_bundle.js') -``` - -The above loads `chart.js` and `graphs_bundle.js` for this page only. `chart.js` -is separated from the bundle file so it can be cached separately from the bundle -and reused for other pages that also rely on the library. For an example, see -[this Haml file][page-specific-js-example]. - -### Minimizing page size - -A smaller page size means the page loads faster (especially important on mobile -and poor connections), the page is parsed more quickly by the browser, and less -data is used for users with capped data plans. - -General tips: - -- Don't add new fonts. -- Prefer font formats with better compression, e.g. WOFF2 is better than WOFF, which is better than TTF. -- Compress and minify assets wherever possible (For CSS/JS, Sprockets does this for us). -- If some functionality can reasonably be achieved without adding extra libraries, avoid them. -- Use page-specific JavaScript as described above to dynamically load libraries that are only needed on certain pages. - -## Accessibility - -### Resources - -[Chrome Accessibility Developer Tools][chrome-accessibility-developer-tools] -are useful for testing for potential accessibility problems in GitLab. - -Accessibility best-practices and more in-depth information is available on -[the Audit Rules page][audit-rules] for the Chrome Accessibility Developer Tools. - -## Security - -### Resources - -[Mozilla’s HTTP Observatory CLI][observatory-cli] and the -[Qualys SSL Labs Server Test][qualys-ssl] are good resources for finding -potential problems and ensuring compliance with security best practices. - -<!-- Uncomment these sections when CSP/SRI are implemented. -### Content Security Policy (CSP) - -Content Security Policy is a web standard that intends to mitigate certain -forms of Cross-Site Scripting (XSS) as well as data injection. - -Content Security Policy rules should be taken into consideration when -implementing new features, especially those that may rely on connection with -external services. - -GitLab's CSP is used for the following: - -- Blocking plugins like Flash and Silverlight from running at all on our pages. -- Blocking the use of scripts and stylesheets downloaded from external sources. -- Upgrading `http` requests to `https` when possible. -- Preventing `iframe` elements from loading in most contexts. - -Some exceptions include: - -- Scripts from Google Analytics and Piwik if either is enabled. -- Connecting with GitHub, Bitbucket, GitLab.com, etc. to allow project importing. -- Connecting with Google, Twitter, GitHub, etc. to allow OAuth authentication. - -We use [the Secure Headers gem][secure_headers] to enable Content -Security Policy headers in the GitLab Rails app. - -Some resources on implementing Content Security Policy: - -- [MDN Article on CSP][mdn-csp] -- [GitHub’s CSP Journey on the GitHub Engineering Blog][github-eng-csp] -- The Dropbox Engineering Blog's series on CSP: [1][dropbox-csp-1], [2][dropbox-csp-2], [3][dropbox-csp-3], [4][dropbox-csp-4] - -### Subresource Integrity (SRI) - -Subresource Integrity prevents malicious assets from being provided by a CDN by -guaranteeing that the asset downloaded is identical to the asset the server -is expecting. - -The Rails app generates a unique hash of the asset, which is used as the -asset's `integrity` attribute. The browser generates the hash of the asset -on-load and will reject the asset if the hashes do not match. - -All CSS and JavaScript assets should use Subresource Integrity. For implementation details, -see the documentation for [the Sprockets implementation of SRI][sprockets-sri]. - -Some resources on implementing Subresource Integrity: - -- [MDN Article on SRI][mdn-sri] -- [Subresource Integrity on the GitHub Engineering Blog][github-eng-sri] - ---> - -### Including external resources - -External fonts, CSS, and JavaScript should never be used with the exception of -Google Analytics and Piwik - and only when the instance has enabled it. Assets -should always be hosted and served locally from the GitLab instance. Embedded -resources via `iframes` should never be used except in certain circumstances -such as with ReCaptcha, which cannot be used without an `iframe`. - -### Avoiding inline scripts and styles - -In order to protect users from [XSS vulnerabilities][xss], we will disable inline scripts in the future using Content Security Policy. - -While inline scripts can be useful, they're also a security concern. If -user-supplied content is unintentionally left un-sanitized, malicious users can -inject scripts into the web app. - -Inline styles should be avoided in almost all cases, they should only be used -when no alternatives can be found. This allows reusability of styles as well as -readability. - -## Style guides and linting - -See the relevant style guides for our guidelines and for information on linting: - -- [SCSS][scss-style-guide] -- JavaScript - We defer to [AirBnb][airbnb-js-style-guide] on most style-related -conventions and enforce them with eslint. See [our current .eslintrc][eslintrc] -for specific rules and patterns. - -## Testing - -Feature tests need to be written for all new features. Regression tests -also need to be written for all bug fixes to prevent them from occurring -again in the future. - -See [the Testing Standards and Style Guidelines](testing.md) for more -information. - -### Running frontend tests - -`rake karma` runs the frontend-only (JavaScript) tests. -It consists of two subtasks: - -- `rake karma:fixtures` (re-)generates fixtures -- `rake karma:tests` actually executes the tests - -As long as the fixtures don't change, `rake karma:tests` is sufficient -(and saves you some time). - -Please note: Not all of the frontend fixtures are generated. Some are still static -files. These will not be touched by `rake karma:fixtures`. - -## Design Patterns - -### Singletons - -When exactly one object is needed for a given task, prefer to define it as a -`class` rather than as an object literal. Prefer also to explicitly restrict -instantiation, unless flexibility is important (e.g. for testing). - -```javascript -// bad - -gl.MyThing = { - prop1: 'hello', - method1: () => {} -}; - -// good - -class MyThing { - constructor() { - this.prop1 = 'hello'; - } - method1() {} -} - -gl.MyThing = new MyThing(); - -// best - -let singleton; - -class MyThing { - constructor() { - if (!singleton) { - singleton = this; - singleton.init(); - } - return singleton; - } - - init() { - this.prop1 = 'hello'; - } - - method1() {} -} - -gl.MyThing = MyThing; - -``` - -### Manipulating the DOM in a JS Class - -When writing a class that needs to manipulate the DOM guarantee a container option is provided. -This is useful when we need that class to be instantiated more than once in the same page. - -Bad: -```javascript -class Foo { - constructor() { - document.querySelector('.bar'); - } -} -new Foo(); -``` - -Good: -```javascript -class Foo { - constructor(opts) { - document.querySelector(`${opts.container} .bar`); - } -} - -new Foo({ container: '.my-element' }); -``` -You can find an example of the above in this [class][container-class-example]; - -## Supported browsers - -For our currently-supported browsers, see our [requirements][requirements]. - - -## Gotchas - -### Spec errors due to use of ES6 features in `.js` files - -If you see very generic JavaScript errors (e.g. `jQuery is undefined`) being -thrown in Karma, Spinach, or Rspec tests but can't reproduce them manually, -you may have included `ES6`-style JavaScript in files that don't have the -`.js.es6` file extension. Either use ES5-friendly JavaScript or rename the file -you're working in (`git mv <file.js> <file.js.es6>`). - -### Spec errors due to use of unsupported JavaScript - -Similar errors will be thrown if you're using JavaScript features not yet -supported by our test runner's version of webkit, whether or not you've updated -the file extension. Examples of unsupported JavaScript features are: - -- Array.from -- Array.find -- Array.first -- Object.assign -- Async functions -- Generators -- Array destructuring -- For Of -- Symbol/Symbol.iterator -- Spread - -Until these are polyfilled or transpiled appropriately, they should not be used. -Please update this list with additional unsupported features or when any of -these are made usable. - -### Spec errors due to JavaScript not enabled - -If, as a result of a change you've made, a feature now depends on JavaScript to -run correctly, you need to make sure a JavaScript web driver is enabled when -specs are run. If you don't you'll see vague error messages from the spec -runner, and an explosion of vague console errors in the HTML snapshot. - -To enable a JavaScript driver in an `rspec` test, add `js: true` to the -individual spec or the context block containing multiple specs that need -JavaScript enabled: - -```ruby - -# For one spec -it 'presents information about abuse report', js: true do - # assertions... -end - -describe "Admin::AbuseReports", js: true do - it 'presents information about abuse report' do - # assertions... - end - it 'shows buttons for adding to abuse report' do - # assertions... - end -end -``` - -In Spinach, the JavaScript driver is enabled differently. In the `*.feature` -file for the failing spec, add the `@javascript` flag above the Scenario: - -``` -@javascript -Scenario: Developer can approve merge request - Given I am a "Shop" developer - And I visit project "Shop" merge requests page - And merge request 'Bug NS-04' must be approved - And I click link "Bug NS-04" - When I click link "Approve" - Then I should see approved merge request "Bug NS-04" - -``` +# Frontend Development Guidelines -[rails]: http://rubyonrails.org/ -[haml]: http://haml.info/ -[hamlit]: https://github.com/k0kubun/hamlit -[hamlit-limits]: https://github.com/k0kubun/hamlit/blob/master/REFERENCE.md#limitations -[scss]: http://sass-lang.com/ -[es6]: https://babeljs.io/ -[sprockets]: https://github.com/rails/sprockets -[jquery]: https://jquery.com/ -[vue]: http://vuejs.org/ -[vue-docs]: http://vuejs.org/guide/index.html -[web-page-test]: http://www.webpagetest.org/ -[pagespeed-insights]: https://developers.google.com/speed/pagespeed/insights/ -[google-devtools-profiling]: https://developers.google.com/web/tools/chrome-devtools/profile/?hl=en -[browser-diet]: https://browserdiet.com/ -[d3]: https://d3js.org/ -[chartjs]: http://www.chartjs.org/ -[page-specific-js-example]: https://gitlab.com/gitlab-org/gitlab-ce/blob/13bb9ed77f405c5f6ee4fdbc964ecf635c9a223f/app/views/projects/graphs/_head.html.haml#L6-8 -[chrome-accessibility-developer-tools]: https://github.com/GoogleChrome/accessibility-developer-tools -[audit-rules]: https://github.com/GoogleChrome/accessibility-developer-tools/wiki/Audit-Rules -[observatory-cli]: https://github.com/mozilla/http-observatory-cli -[qualys-ssl]: https://www.ssllabs.com/ssltest/analyze.html -[secure_headers]: https://github.com/twitter/secureheaders -[mdn-csp]: https://developer.mozilla.org/en-US/docs/Web/Security/CSP -[github-eng-csp]: http://githubengineering.com/githubs-csp-journey/ -[dropbox-csp-1]: https://blogs.dropbox.com/tech/2015/09/on-csp-reporting-and-filtering/ -[dropbox-csp-2]: https://blogs.dropbox.com/tech/2015/09/unsafe-inline-and-nonce-deployment/ -[dropbox-csp-3]: https://blogs.dropbox.com/tech/2015/09/csp-the-unexpected-eval/ -[dropbox-csp-4]: https://blogs.dropbox.com/tech/2015/09/csp-third-party-integrations-and-privilege-separation/ -[mdn-sri]: https://developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integrity -[github-eng-sri]: http://githubengineering.com/subresource-integrity/ -[sprockets-sri]: https://github.com/rails/sprockets-rails#sri-support -[xss]: https://en.wikipedia.org/wiki/Cross-site_scripting -[scss-style-guide]: scss_styleguide.md -[requirements]: ../install/requirements.md#supported-web-browsers -[issue-boards]: https://gitlab.com/gitlab-org/gitlab-ce/tree/master/app/assets/javascripts/boards -[environments-table]: https://gitlab.com/gitlab-org/gitlab-ce/tree/master/app/assets/javascripts/environments -[page_specific_javascript]: https://docs.gitlab.com/ce/development/frontend.html#page-specific-javascript -[component-system]: https://vuejs.org/v2/guide/#Composing-with-Components -[state-management]: https://vuejs.org/v2/guide/state-management.html#Simple-State-Management-from-Scratch -[vue-resource-repo]: https://github.com/pagekit/vue-resource -[issue-boards-service]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/assets/javascripts/boards/services/board_service.js.es6 -[airbnb-js-style-guide]: https://github.com/airbnb/javascript -[eslintrc]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/.eslintrc -[team-page]: https://about.gitlab.com/team -[vue-section]: https://docs.gitlab.com/ce/development/frontend.html#how-to-build-a-new-feature-with-vue-js -[container-class-example]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/assets/javascripts/mini_pipeline_graph_dropdown.js +This page has moved [here](fe_guide/index.md). diff --git a/doc/development/rake_tasks.md b/doc/development/rake_tasks.md index dcd978c4cd3..ec9e4dcc59d 100644 --- a/doc/development/rake_tasks.md +++ b/doc/development/rake_tasks.md @@ -42,6 +42,20 @@ To run several tests inside one directory: If you want to use [Spring](https://github.com/rails/spring) set `ENABLE_SPRING=1` in your environment. +## Compile Frontend Assets + +You shouldn't ever need to compile frontend assets manually in development, but +if you ever need to test how the assets get compiled in a production +environment you can do so with the following command: + +``` +RAILS_ENV=production NODE_ENV=production bundle exec rake gitlab:assets:compile +``` + +This will compile and minify all JavaScript and CSS assets and copy them along +with all other frontend assets (images, fonts, etc) into `/public/assets` where +they can be easily inspected. + ## Generate API documentation for project services (e.g. Slack) ``` diff --git a/doc/development/testing.md b/doc/development/testing.md index 5ac7b8dadeb..5bc958f5a96 100644 --- a/doc/development/testing.md +++ b/doc/development/testing.md @@ -34,16 +34,17 @@ GitLab uses [factory_girl] as a test fixture replacement. GitLab uses [Karma] to run its [Jasmine] JavaScript specs. They can be run on the command line via `bundle exec karma`. -- JavaScript tests live in `spec/javascripts/`, matching the folder structure of - `app/assets/javascripts/`: `app/assets/javascripts/behaviors/autosize.js` has a corresponding - `spec/javascripts/behaviors/autosize_spec.js` file. +- JavaScript tests live in `spec/javascripts/`, matching the folder structure + of `app/assets/javascripts/`: `app/assets/javascripts/behaviors/autosize.js` + has a corresponding `spec/javascripts/behaviors/autosize_spec.js` file. - Haml fixtures required for JavaScript tests live in `spec/javascripts/fixtures`. They should contain the bare minimum amount of markup necessary for the test. > **Warning:** Keep in mind that a Rails view may change and invalidate your test, but everything will still pass because your fixture - doesn't reflect the latest view. + doesn't reflect the latest view. Because of this we encourage you to + generate fixtures from actual rails views whenever possible. - Keep in mind that in a CI environment, these tests are run in a headless browser and you will not have access to certain APIs, such as @@ -53,6 +54,8 @@ the command line via `bundle exec karma`. [Karma]: https://github.com/karma-runner/karma [Jasmine]: https://github.com/jasmine/jasmine +For more information, see the [frontend testing guide](fe_guide/testing.md). + ## RSpec ### General Guidelines |