diff options
Diffstat (limited to 'doc/development/fe_guide')
29 files changed, 679 insertions, 280 deletions
diff --git a/doc/development/fe_guide/accessibility.md b/doc/development/fe_guide/accessibility.md index 92730e8139f..5ad1a701fac 100644 --- a/doc/development/fe_guide/accessibility.md +++ b/doc/development/fe_guide/accessibility.md @@ -9,11 +9,9 @@ info: To determine the technical writer assigned to the Stage/Group associated w ## Resources [Chrome Accessibility Developer Tools](https://github.com/GoogleChrome/accessibility-developer-tools) -are useful for testing for potential accessibility problems in GitLab. +assist with testing for potential accessibility problems in GitLab. -The [axe](https://www.deque.com/axe/) browser extension (available for [Firefox](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/) and [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd)) is -also a handy tool for running audits and getting feedback on markup, CSS and even potentially problematic color usages. +The [axe](https://www.deque.com/axe/) browser extension (available for [Firefox](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/) and [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd)) provides running audits and feedback on markup, CSS, and even potentially problematic color usages. Accessibility best-practices and more in-depth information are available on -[the Audit Rules page](https://github.com/GoogleChrome/accessibility-developer-tools/wiki/Audit-Rules) for the Chrome Accessibility Developer Tools. The [Awesome Accessibility](https://github.com/brunopulis/awesome-a11y) list is also a -useful compilation of accessibility-related material. +[the Audit Rules page](https://github.com/GoogleChrome/accessibility-developer-tools/wiki/Audit-Rules) for the Chrome Accessibility Developer Tools. The [Awesome Accessibility](https://github.com/brunopulis/awesome-a11y) list is a compilation of accessibility-related material. diff --git a/doc/development/fe_guide/architecture.md b/doc/development/fe_guide/architecture.md index 964837dc5f7..c51f99ca9d2 100644 --- a/doc/development/fe_guide/architecture.md +++ b/doc/development/fe_guide/architecture.md @@ -6,9 +6,7 @@ info: To determine the technical writer assigned to the Stage/Group associated w # 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. +When developing a feature that requires architectural design, or changing the fundamental design of an existing feature, discuss it with a Frontend Architecture Expert. A Frontend Architect is an expert who makes high-level Frontend design decisions and decides on technical standards, including coding standards and frameworks. diff --git a/doc/development/fe_guide/axios.md b/doc/development/fe_guide/axios.md index cf5a4970c04..2d699b305ce 100644 --- a/doc/development/fe_guide/axios.md +++ b/doc/development/fe_guide/axios.md @@ -44,7 +44,7 @@ Advantages over [`spyOn()`](https://jasmine.github.io/api/edge/global.html#spyOn - no need to create response objects - does not allow call through (which we want to avoid) -- simple API to test error cases +- clear API to test error cases - provides `replyOnce()` to allow for different responses We have also decided against using [Axios interceptors](https://github.com/axios/axios#interceptors) because they are not suitable for mocking. diff --git a/doc/development/fe_guide/components.md b/doc/development/fe_guide/components.md deleted file mode 100644 index 6b6274a6480..00000000000 --- a/doc/development/fe_guide/components.md +++ /dev/null @@ -1,3 +0,0 @@ ---- -redirect_to: 'https://design.gitlab.com/components/status/' ---- diff --git a/doc/development/fe_guide/dark_mode.md b/doc/development/fe_guide/dark_mode.md new file mode 100644 index 00000000000..dd7ffd1ee6c --- /dev/null +++ b/doc/development/fe_guide/dark_mode.md @@ -0,0 +1,77 @@ +--- +type: reference, dev +stage: none +group: Development +info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/engineering/ux/technical-writing/#assignments +--- + +This page is about developing dark mode for GitLab. We also have documentation on how +[to enable dark mode](../../user/profile/preferences.md#dark-mode). + +# How dark mode works + +Short version: Reverse the color palette and override a few Bootstrap variables. + +Note the following: + +- The dark mode palette is defined in `app/assets/stylesheets/themes/_dark.scss`. + This is loaded _before_ application.scss to generate `application_dark.css` + - We define two types of variables in `_dark.scss`: + - SCSS variables are used in framework, components, and utitlity classes. + - CSS variables are used for any colors within the `app/assets/stylesheets/page_bundles` directory. +- `app/views/layouts/_head.html.haml` then loads application or application_dark based on the user's theme preference. + +As we do not want to generate separate `_dark.css` variants of every page_bundle file, +we use CSS variables with SCSS variables as fallbacks. This is because when we generate the `page_bundles` +CSS, we get the variable values from `_variables.scss`, so any SCSS variables have light mode values. + +As the CSS variables defined in `_dark.scss` are available in the browser, they have the +correct colors for dark mode. + +```scss +color: var(--gray-500, $gray-500); +``` + +## Utility classes + +We generate a separate `utilities_dark.css` file for utility classes containing the inverted values. So a class +such as `gl-text-white` specifies a text color of `#333` in dark mode. This means you do not have to +add multiple classes every time you want to add a color. + +Currently, we cannot set up a utility class only in dark mode. We hope to address that +[issue](https://gitlab.com/gitlab-org/gitlab-ui/-/issues/1141) soon. + +## Using different values in light and dark mode + +In most cases, we can use the same values for light and dark mode. If that is not possible, you +can add an override using the `.gl-dark` class that dark mode adds to `body`: + +```scss +color: $gray-700; +.gl-dark & { + color: var(--gray-500); +} +``` + +NOTE: +Avoid using a different value for the SCSS fallback + +```scss +// avoid where possible +// --gray-500 (#999) in dark mode +// $gray-700 (#525252) in light mode +color: var(--gray-500, $gray-700); +``` + +We [plan to add](https://gitlab.com/gitlab-org/gitlab/-/issues/301147) the CSS variables to light mode. When that happens, different values for the SCSS fallback will no longer work. + +## When to use SCSS variables + +There are a few things we do in SCSS that we cannot (easily) do with CSS, such as the following +functions: + +- `lighten` +- `darken` +- `color-yiq` (color contrast) + +If those are needed then SCSS variables should be used. diff --git a/doc/development/fe_guide/dependencies.md b/doc/development/fe_guide/dependencies.md index b036819cde1..8fe03544f85 100644 --- a/doc/development/fe_guide/dependencies.md +++ b/doc/development/fe_guide/dependencies.md @@ -6,28 +6,75 @@ info: To determine the technical writer assigned to the Stage/Group associated w # Frontend dependencies -## Package manager +We use [yarn@1](https://classic.yarnpkg.com/lang/en/) to manage frontend dependencies. -We use [Yarn](https://yarnpkg.com/) to manage frontend dependencies. There are a few exceptions, stored in `vendor/assets/`. +There are a few exceptions in the GitLab repository, stored in `vendor/assets/`. -## Updating dependencies +## What are production and development dependencies? + +These dependencies are defined in two groups within `package.json`, `dependencies` and `devDependencies`. +For our purposes, we consider anything that is required to compile our production assets a "production" dependency. +That is, anything required to run the `webpack` script with `NODE_ENV=production`. +Tools like `eslint`, `jest`, and various plugins and tools used in development are considered `devDependencies`. +This distinction is used by omnibus to determine which dependencies it requires when building GitLab. -### Renovate GitLab Bot +Exceptions are made for some tools that we require in the +`compile-production-assets` CI job such as `webpack-bundle-analyzer` to analyze our +production assets post-compile. + +## Updating dependencies We use the [Renovate GitLab Bot](https://gitlab.com/gitlab-org/frontend/renovate-gitlab-bot) to -automatically create merge requests for updating dependencies of several projects. You can find the -up-to-date list of projects managed by the renovate bot in the project’s README. Some key dependencies -updated using renovate are: +automatically create merge requests for updating dependencies of several projects. +You can find the up-to-date list of projects managed by the renovate bot in the project’s README. + +Some key dependencies updated using renovate are: - [`@gitlab/ui`](https://gitlab.com/gitlab-org/gitlab-ui) - [`@gitlab/svgs`](https://gitlab.com/gitlab-org/gitlab-svgs) - [`@gitlab/eslint-plugin`](https://gitlab.com/gitlab-org/frontend/eslint-plugin) +- And any other package in the `@gitlab/` scope + +We have the goal of updating [_all_ dependencies with renovate](https://gitlab.com/gitlab-org/frontend/rfcs/-/issues/21). + +Updating dependencies automatically has several benefits, have a look at this [example MR](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/53613). + +- MRs will be created automatically when new versions are released +- MRs can easily be rebased and updated with just checking a checkbox in the MR description +- MRs contain changelog summaries and links to compare the different package versions +- MRs can be assigned to people directly responsible for the dependencies + +### Community contributions updating dependencies + +It is okay to reject Community Contributions that solely bump dependencies. +Simple dependency updates are better done automatically for the reasons provided above. +If a community contribution needs to be rebased, runs into conflicts, or goes stale, the effort required +to instruct the contributor to correct it often outweighs the benefits. + +If a dependency update is accompanied with significant migration efforts, due to major version updates, +a community contribution is acceptable. + +Here is a message you can use to explain to community contributors as to why we reject simple updates: + +```markdown +Hello CONTRIBUTOR! + +Thank you very much for this contribution. It seems like you are doing a "simple" dependency update. + +If a dependency update is as simple as increasing the version number, we'd like a Bot to do this to save you and ourselves some time. + +This has certain benefits as outlined in our <a href="https://docs.gitlab.com/ee/development/fe_guide/dependencies.html#updating-dependencies">Frontend development guidelines</a>. + +You might find that we do not currently update DEPENDENCY automatically, but we are planning to do so in [the near future](https://gitlab.com/gitlab-org/frontend/rfcs/-/issues/21). + +Thank you for understanding, I will close this Merge Request. +/close +``` ### Blocked dependencies -We discourage installing some dependencies in [GitLab repository](https://gitlab.com/gitlab-org/gitlab) -because they can create conflicts in the dependency tree. Blocked dependencies are declared in the -`blockDependencies` property of the GitLab [`package.json` file](https://gitlab.com/gitlab-org/gitlab/-/blob/master/package.json). +We discourage installing some dependencies in [GitLab repository](https://gitlab.com/gitlab-org/gitlab) because they can create conflicts in the dependency tree. +Blocked dependencies are declared in the `blockDependencies` property of the GitLab [`package.json`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/package.json). ## Dependency notes diff --git a/doc/development/fe_guide/design_anti_patterns.md b/doc/development/fe_guide/design_anti_patterns.md new file mode 100644 index 00000000000..d230e413879 --- /dev/null +++ b/doc/development/fe_guide/design_anti_patterns.md @@ -0,0 +1,219 @@ +--- +stage: none +group: unassigned +info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/engineering/ux/technical-writing/#assignments +--- + +# Design Anti-patterns + +Anti-patterns may seem like good approaches at first, but it has been shown that they bring more ills than benefits. These should +generally be avoided. + +Throughout the GitLab codebase, there may be historic uses of these anti-patterns. Please [use discretion](https://about.gitlab.com/handbook/engineering/#balance-refactoring-and-velocity) +when figuring out whether or not to refactor, when touching code that uses one of these legacy patterns. + +**Please note:** For new features, anti-patterns are not necessarily prohibited, but it is **strongly suggested** to find another approach. + +## Shared Global Object (Anti-pattern) + +A shared global object is an instance of something that can be accessed from anywhere and therefore has no clear owner. + +Here's an example of this pattern applied to a Vuex Store: + +```javascript +const createStore = () => new Vuex.Store({ + actions, + state, + mutations +}); + +// Notice that we are forcing all references to this module to use the same single instance of the store. +// We are also creating the store at import-time and there is nothing which can automatically dispose of it. +// +// As an alternative, we should simply export the `createStore` and let the client manage the +// lifecycle and instance of the store. +export default createStore(); +``` + +### What problems do Shared Global Objects cause? + +Shared Global Objects are convenient because they can be accessed from anywhere. However, +the convenience does not always outweigh their heavy cost: + +- **No ownership.** There is no clear owner to these objects and therefore they assume a non-deterministic + and permanent lifecycle. This can be especially problematic for tests. +- **No access control.** When Shared Global Objects manage some state, this can create some very buggy and difficult + coupling situations because there is no access control to this object. +- **Possible circular references.** Shared Global Objects can also create some circular referencing situations since submodules + of the Shared Global Object can reference modules that reference itself (see + [this MR for an example](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/33366)). + +Here are some historic examples where this pattern was identified to be problematic: + +- [Reference to global Vuex store in IDE](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/36401) +- [Docs update to discourage singleton Vuex store](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/36952) + +### When could the Shared Global Object pattern be actually appropriate? + +Shared Global Object's solve the problem of making something globally accessible. This pattern +could be appropriate: + +- When a responsibility is truly global and should be referenced across the application + (e.g., an application-wide Event Bus). + +Even in these scenarios, please consider avoiding the Shared Global Object pattern because the +side-effects can be notoriously difficult to reason with. + +### References + +To read more on this topic, check out the following references: + +- [GlobalVariablesAreBad from C2 wiki](https://wiki.c2.com/?GlobalVariablesAreBad) + +## Singleton (Anti-pattern) + +The classic [Singleton pattern](https://en.wikipedia.org/wiki/Singleton_pattern) is an approach to ensure that only one +instance of a thing exists. + +Here's an example of this pattern: + +```javascript +class MyThing { + constructor() { + // ... + } + + // ... +} + +MyThing.instance = null; + +export const getThingInstance = () => { + if (MyThing.instance) { + return MyThing.instance; + } + + const instance = new MyThing(); + MyThing.instance = instance; + return instance; +}; +``` + +### What problems do Singletons cause? + +It is a big assumption that only one instance of a thing should exist. More often than not, +a Singleton is misused and causes very tight coupling amongst itself and the modules that reference it. + +Here are some historic examples where this pattern was identified to be problematic: + +- [Test issues caused by singleton class in IDE](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/30398#note_331174190) +- [Implicit Singleton created by module's shared variables](https://gitlab.com/gitlab-org/gitlab-vscode-extension/-/merge_requests/97#note_417515776) +- [Complexity caused by Singletons](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/29461#note_324585814) + +Here are some ills that Singletons often produce: + +1. **Non-deterministic tests.** Singletons encourage non-deterministic tests because the single instance is shared across + individual tests, often causing the state of one test to bleed into another. +1. **High coupling.** Under the hood, clients of a singleton class all share a single specific + instance of an object, which means this pattern inherits all the [problems of Shared Global Object](#what-problems-do-shared-global-objects-cause) + such as no clear ownership and no access control. These leads to high coupling situations that can + be buggy and difficult to untangle. +1. **Infectious.** Singletons are infectious, especially when they manage state. Consider the component + [RepoEditor](https://gitlab.com/gitlab-org/gitlab/blob/27ad6cb7b76430fbcbaf850df68c338d6719ed2b/app%2Fassets%2Fjavascripts%2Fide%2Fcomponents%2Frepo_editor.vue#L0-1) + used in the Web IDE. This component interfaces with a Singleton [Editor](https://gitlab.com/gitlab-org/gitlab/blob/862ad57c44ec758ef3942ac2e7a2bd40a37a9c59/app%2Fassets%2Fjavascripts%2Fide%2Flib%2Feditor.js#L21) + which manages some state for working with Monaco. Because of the Singleton nature of the Editor class, + the component `RepoEditor` is now forced to be a Singleton as well. Multiple instances of this component + would cause production issues because no one truly owns the instance of `Editor`. + +### Why is the Singleton pattern popular in other languages like Java? + +This is because of the limitations of languages like Java where everything has to be wrapped +in a class. In JavaScript we have things like object and function literals where we can solve +many problems with a module that simply exports utility functions. + +### When could the Singleton pattern be actually appropriate?** + +Singletons solve the problem of enforcing there to be only 1 instance of a thing. It's possible +that a Singleton could be appropriate in the following rare cases: + +- We need to manage some resource that **MUST** have just 1 instance (i.e. some hardware restriction). +- There is a real [cross-cutting concern](https://en.wikipedia.org/wiki/Cross-cutting_concern) (e.g., logging) and a Singleton provides the simplest API. + +Even in these scenarios, please consider avoiding the Singleton pattern. + +### What alternatives are there to the Singleton pattern? + +#### Utility Functions + +When no state needs to be managed, we can simply export utility functions from a module without +messing with any class instantiation. + +```javascript +// bad - Singleton +export class ThingUtils { + static create() { + if(this.instance) { + return this.instance; + } + + this.instance = new ThingUtils(); + return this.instance; + } + + bar() { /* ... */ } + + fuzzify(id) { /* ... */ } +} + +// good - Utility functions +export const bar = () => { /* ... */ }; + +export const fuzzify = (id) => { /* ... */ }; +``` + +#### Dependency Injection + +[Dependency Injection](https://en.wikipedia.org/wiki/Dependency_injection) is an approach which breaks +coupling by declaring a module's dependencies to be injected from outside the module (e.g., through constructor parameters, a bona-fide Dependency Injection framework, and even Vue's `provide/inject`). + +```javascript +// bad - Vue component coupled to Singleton +export default { + created() { + this.mediator = MyFooMediator.getInstance(); + }, +}; + +// good - Vue component declares dependency +export default { + inject: ['mediator'] +}; +``` + +```javascript +// bad - We're not sure where the singleton is in it's lifecycle so we init it here. +export class Foo { + constructor() { + Bar.getInstance().init(); + } + + stuff() { + return Bar.getInstance().doStuff(); + } +} + +// good - Lets receive this dependency as a constructor argument. +// It's also not our responsibility to manage the lifecycle. +export class Foo { + constructor(bar) { + this.bar = bar; + } + + stuff() { + return this.bar.doStuff(); + } +} +``` + +In this example, the lifecycle and implementation details of `mediator` are all managed +**outside** the component (most likely the page entrypoint). diff --git a/doc/development/fe_guide/design_patterns.md b/doc/development/fe_guide/design_patterns.md index 784612682f8..c769d0767e7 100644 --- a/doc/development/fe_guide/design_patterns.md +++ b/doc/development/fe_guide/design_patterns.md @@ -6,79 +6,11 @@ info: To determine the technical writer assigned to the Stage/Group associated w # Design Patterns -## Singletons +The following design patterns are suggested approaches for solving common problems. Use discretion when evaluating +if a certain pattern makes sense in your situation. Just because it is a pattern, doesn't mean it is a good one for your problem. -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). +**Please note:** When adding a design pattern to this document, be sure to clearly state the **problem it solves**. -```javascript -// bad +## TBD -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 (!MyThing.prototype.singleton) { - this.init(); - MyThing.prototype.singleton = this; - } - return MyThing.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](https://gitlab.com/gitlab-org/gitlab/blob/master/app/assets/javascripts/mini_pipeline_graph_dropdown.js); +Stay tuned! diff --git a/doc/development/fe_guide/development_process.md b/doc/development/fe_guide/development_process.md index d122459f51c..b85ed4da442 100644 --- a/doc/development/fe_guide/development_process.md +++ b/doc/development/fe_guide/development_process.md @@ -12,9 +12,9 @@ You can find more about the organization of the frontend team in the [handbook]( The idea is to remind us about specific topics during the time we build a new feature or start something. This is a common practice in other industries (like pilots) that also use standardized checklists to reduce problems early on. -Copy the content over to your issue or merge request and if something doesn't apply simply remove it from your current list. +Copy the content over to your issue or merge request and if something doesn't apply, remove it from your current list. -This checklist is intended to help us during development of bigger features/refactorings, it's not a "use it always and every point always matches" list. +This checklist is intended to help us during development of bigger features/refactorings. It is not a "use it always and every point always matches" list. Please use your best judgment when to use it and please contribute new points through merge requests if something comes to your mind. @@ -77,7 +77,7 @@ With the purpose of being [respectful of others' time](https://about.gitlab.com/ - includes tests - includes a changelog entry (when necessary) - Before assigning to a maintainer, assign to a reviewer. -- If you assigned a merge request or pinged someone directly, be patient because we work in different timezones and asynchronously. Unless the merge request is urgent (like fixing a broken master), please don't DM or reassign the merge request before waiting for a 24-hour window. +- If you assigned a merge request or pinged someone directly, be patient because we work in different timezones and asynchronously. Unless the merge request is urgent (like fixing a broken default branch), please don't DM or reassign the merge request before waiting for a 24-hour window. - If you have a question regarding your merge request/issue, make it on the merge request/issue. When we DM each other, we no longer have a SSOT and [no one else is able to contribute](https://about.gitlab.com/handbook/values/#public-by-default). - When you have a big **Draft** merge request with many changes, you're advised to get the review started before adding/removing significant code. Make sure it is assigned well before the release cut-off, as the reviewer(s)/maintainer(s) would always prioritize reviewing finished MRs before the **Draft** ones. - Make sure to remove the `Draft:` title before the last round of review. diff --git a/doc/development/fe_guide/dropdowns.md b/doc/development/fe_guide/dropdowns.md deleted file mode 100644 index bd2dae13c5b..00000000000 --- a/doc/development/fe_guide/dropdowns.md +++ /dev/null @@ -1,3 +0,0 @@ ---- -redirect_to: 'https://design.gitlab.com/components/dropdown/' ---- diff --git a/doc/development/fe_guide/editor_lite.md b/doc/development/fe_guide/editor_lite.md index 47ef85d8737..f783a97fbd3 100644 --- a/doc/development/fe_guide/editor_lite.md +++ b/doc/development/fe_guide/editor_lite.md @@ -8,13 +8,13 @@ info: To determine the technical writer assigned to the Stage/Group associated w ## Background -**Editor Lite** is a technological product driving [Web Editor](../../user/project/repository/web_editor.md), [Snippets](../../user/snippets.md), [CI Linter](../../ci/lint.md), etc. Editor Lite is the driving technology for any single-file editing experience across the product. +**Editor Lite** is a technological product driving features like [Web Editor](../../user/project/repository/web_editor.md), [Snippets](../../user/snippets.md), and [CI Linter](../../ci/lint.md). Editor Lite is the driving technology for any single-file editing experience across the product. Editor Lite is a thin wrapper around [the Monaco editor](https://microsoft.github.io/monaco-editor/index.html) that provides the necessary helpers and abstractions and extends Monaco using extensions. ## How to use Editor Lite -Editor Lite is framework-agnostic and can be used in any application, whether it's Rails or Vue. For the convenience of integration, we have [the dedicated `<editor-lite>` Vue component](#vue-component), but in general, the integration of Editor Lite is pretty straightforward: +Editor Lite is framework-agnostic and can be used in any application, whether it's Rails or Vue. For the convenience of integration, we have the dedicated `<editor-lite>` Vue component, but in general, the integration of Editor Lite is pretty straightforward: 1. Import Editor Lite: @@ -65,7 +65,7 @@ The editor follows the same public API as [provided by Monaco editor](https://mi 1. Editor's loading state. -Editor Lite comes with the loading state built-in, making spinners and loaders rarely needed in HTML. To benefit the built-in loading state, set the `data-editor-loading` property on the HTML element that is supposed to contain the editor. Editor Lite will show the loader automatically while it's bootstrapping. +Editor Lite comes with the loading state built-in, making spinners and loaders rarely needed in HTML. To benefit the built-in loading state, set the `data-editor-loading` property on the HTML element that is supposed to contain the editor. Editor Lite shows the loader automatically while it's bootstrapping. ![Editor Lite: loading state](img/editor_lite_loading.png) 1. Update syntax highlighting if the filename changes. @@ -89,7 +89,7 @@ form.addEventListener('submit', () => { 1. Performance -Even though Editor Lite itself is extremely slim, it still depends on Monaco editor. Monaco is not an easily tree-shakeable module. Hence, every time you add Editor Lite to a view, the JavaScript bundle's size significantly increases, affecting your view's loading performance. To avoid that, it is recommended to import the editor on demand on those views where it is not 100% certain that the editor will be used. Or if the editor is a secondary element of the view. Loading Editor Lite on demand is no different from loading any other module: +Even though Editor Lite itself is extremely slim, it still depends on Monaco editor. Monaco is not an easily tree-shakeable module. Hence, every time you add Editor Lite to a view, the JavaScript bundle's size significantly increases, affecting your view's loading performance. It is recommended to import the editor on demand on those views where it is not 100% certain that the editor is needed. Or if the editor is a secondary element of the view. Loading Editor Lite on demand is no different from loading any other module: ```javascript someActionFunction() { @@ -109,8 +109,8 @@ which would not depend on any particular group. Even though the Editor Lite's co [Create::Editor FE Team](https://about.gitlab.com/handbook/engineering/development/dev/create-editor/), the main functional elements — extensions — can be owned by any group. Editor Lite extensions' main idea is that the core of the editor remains very slim and stable. At the same time, whatever new functionality -is needed can be added as an extension to this core, without touching the core itself. It allows any group -to build and own any new editing functionality without being afraid of it being broken or overridden with +is needed can be added as an extension to this core, without touching the core itself. Any group is allowed +to build and own new editing functionality without being afraid of it being broken or overridden with the Editor Lite changes. Structurally, the complete implementation of Editor Lite could be presented as the following diagram: @@ -145,7 +145,7 @@ Important things to note here: ### Using an existing extension -Adding an extension to Editor Lite's instance is simple: +Adding an extension to Editor Lite's instance requires the following steps: ```javascript import EditorLite from '~/editor/editor_lite'; @@ -159,7 +159,7 @@ editor.use(MyExtension); ### Creating an extension -Let's create our first Editor Lite extension. As aforementioned, extensions are ES6 modules exporting the simple `Object` that is used to extend Editor Lite's functionality. As the most straightforward test, let's create an extension that extends Editor Lite with a new function that, when called, will output editor's content in `alert`. +Let's create our first Editor Lite extension. Extensions are ES6 modules exporting a basic `Object` that is used to extend Editor Lite's functionality. As a test, let's create an extension that extends Editor Lite with a new function that, when called, outputs editor's content in `alert`. `~/my_folder/my_fancy_extension.js:` @@ -225,7 +225,3 @@ Just pass the array of extensions to your `use` method: ```javascript editor.use([FileTemplateExtension, MyFancyExtension]); ``` - -## <a id="vue-component"></a>`<editor-lite>` Vue component - -TBD diff --git a/doc/development/fe_guide/emojis.md b/doc/development/fe_guide/emojis.md index 7151e2ffeee..2dedbc8f19d 100644 --- a/doc/development/fe_guide/emojis.md +++ b/doc/development/fe_guide/emojis.md @@ -25,7 +25,7 @@ when your platform does not support it. - `app/assets/images/emoji.png` - `app/assets/images/emoji@2x.png` 1. Ensure you see new individual images copied into `app/assets/images/emoji/` - 1. Ensure you can see the new emojis and their aliases in the GFM Autocomplete + 1. Ensure you can see the new emojis and their aliases in the GitLab Flavored Markdown (GFM) Autocomplete 1. Ensure you can see the new emojis and their aliases in the award emoji menu 1. You might need to add new emoji Unicode support checks and rules for platforms that do not support a certain emoji and we need to fallback to an image. diff --git a/doc/development/fe_guide/frontend_faq.md b/doc/development/fe_guide/frontend_faq.md index 9612f604b56..bf1dae6e7bd 100644 --- a/doc/development/fe_guide/frontend_faq.md +++ b/doc/development/fe_guide/frontend_faq.md @@ -21,7 +21,7 @@ info: To determine the technical writer assigned to the Stage/Group associated w ## FAQ -### 1. How do I find the Rails route for a page? +### 1. How does one find the Rails route for a page? #### Check the 'page' data attribute @@ -36,7 +36,7 @@ Find here the [source code setting the attribute](https://gitlab.com/gitlab-org/ #### Rails routes -The `rake routes` command can be used to list all the routes available in the application, piping the output into `grep`, we can perform a search through the list of available routes. +The `rake routes` command can be used to list all the routes available in the application. Piping the output into `grep`, we can perform a search through the list of available routes. The output includes the request types available, route parameters and the relevant controller. ```shell @@ -46,13 +46,13 @@ bundle exec rake routes | grep "issues" ### 2. `modal_copy_button` vs `clipboard_button` The `clipboard_button` uses the `copy_to_clipboard.js` behavior, which is -initialized on page load, so if there are vue-based clipboard buttons that -don't exist at page load (such as ones in a `GlModal`), they do not have the +initialized on page load. Vue clipboard buttons that +don't exist at page load (such as ones in a `GlModal`) do not have click handlers associated with the clipboard package. -`modal_copy_button` was added that manages an instance of the +`modal_copy_button` manages an instance of the [`clipboard` plugin](https://www.npmjs.com/package/clipboard) specific to -the instance of that component, which means that clipboard events are +the instance of that component. This means that clipboard events are bound on mounting and destroyed when the button is, mitigating the above issue. It also has bindings to a particular container or modal ID available, to work with the focus trap created by our GlModal. @@ -60,7 +60,7 @@ available, to work with the focus trap created by our GlModal. ### 3. A `gitlab-ui` component not conforming to [Pajamas Design System](https://design.gitlab.com/) Some [Pajamas Design System](https://design.gitlab.com/) components implemented in -`gitlab-ui` do not conform with the design system specs because they lack some +`gitlab-ui` do not conform with the design system specs. This is because they lack some planned features or are not correctly styled yet. In the Pajamas website, a banner on top of the component examples indicates that: @@ -77,18 +77,17 @@ It makes codebase unified and more comfortable to maintain/refactor in the futur Ensure a [Product Designer](https://about.gitlab.com/company/team/?department=ux-department) reviews the use of the non-conforming component as part of the MR review. Make a -follow up issue and attach it to the component implementation epic found within the +follow up issue and attach it to the component implementation epic found in the [Components of Pajamas Design System epic](https://gitlab.com/groups/gitlab-org/-/epics/973). ### 4. My submit form button becomes disabled after submitting -If you are using a submit button inside a form and you attach an `onSubmit` event listener on the form element, [this piece of code](https://gitlab.com/gitlab-org/gitlab/blob/794c247a910e2759ce9b401356432a38a4535d49/app/assets/javascripts/main.js#L225) adds a `disabled` class selector to the submit button when the form is submitted. -To avoid this behavior, add the class `js-no-auto-disable` to the button. +A Submit button inside of a form attaches an `onSubmit` event listener on the form element. [This code](https://gitlab.com/gitlab-org/gitlab/blob/794c247a910e2759ce9b401356432a38a4535d49/app/assets/javascripts/main.js#L225) adds a `disabled` class selector to the submit button when the form is submitted. To avoid this behavior, add the class `js-no-auto-disable` to the button. -### 5. Should I use a full URL (i.e. `gon.gitlab_url`) or a full path (i.e. `gon.relative_url_root`) when referencing backend endpoints? +### 5. Should one use a full URL (for example `gon.gitlab_url`) or a full path (for example `gon.relative_url_root`) when referencing backend endpoints? -It's preferred to use a **full path** over a **full URL** because the URL uses the hostname configured with -GitLab which may not match the request. This causes [CORS issues like this Web IDE one](https://gitlab.com/gitlab-org/gitlab/-/issues/36810). +It's preferred to use a **full path** over a **full URL**. This is because the URL uses the hostname configured with +GitLab which may not match the request. This causes [cross-origin resource sharing issues like this Web IDE example](https://gitlab.com/gitlab-org/gitlab/-/issues/36810). Example: @@ -117,7 +116,7 @@ Example: ### 6. How should the Frontend reference Backend paths? -We prefer not to add extra coupling by hardcoding paths. If possible, +We prefer not to add extra coupling by hard-coding paths. If possible, add these paths as data attributes to the DOM element being referenced in the JavaScript. Example: @@ -153,7 +152,7 @@ export const fetchFoos = ({ state }) => { }; ``` -### 7. How can I test the production build locally? +### 7. How can one test the production build locally? Sometimes it's necessary to test locally what the frontend production build would produce, to do so the steps are: @@ -161,7 +160,7 @@ Sometimes it's necessary to test locally what the frontend production build woul 1. Open `gitlab.yaml` located in your `gitlab` installation folder, scroll down to the `webpack` section and change `dev_server` to `enabled: false`. 1. Run `yarn webpack-prod && gdk restart rails-web`. -The production build takes a few minutes to be completed; any code changes at this point are +The production build takes a few minutes to be completed. Any code changes at this point are displayed only after executing the item 3 above again. To return to the normal development mode: @@ -176,8 +175,8 @@ To return to the normal development mode: > [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/28837) in GitLab 12.8. GitLab has enabled the Babel `preset-env` option -[`useBuiltIns: 'usage'`](https://babeljs.io/docs/en/babel-preset-env#usebuiltins-usage), -which adds the appropriate `core-js` polyfills once for each JavaScript feature +[`useBuiltIns: 'usage'`](https://babeljs.io/docs/en/babel-preset-env#usebuiltins-usage). +This adds the appropriate `core-js` polyfills once for each JavaScript feature we're using that our target browsers don't support. You don't need to add `core-js` polyfills manually. @@ -199,3 +198,7 @@ To see what polyfills are being used: which polyfills are being loaded and where: ![Image of webpack report](img/webpack_report_v12_8.png) + +### 9. Why is my page broken in dark mode? + +See [dark mode docs](dark_mode.md) diff --git a/doc/development/fe_guide/graphql.md b/doc/development/fe_guide/graphql.md index cbaa648570c..a53d9fee029 100644 --- a/doc/development/fe_guide/graphql.md +++ b/doc/development/fe_guide/graphql.md @@ -18,34 +18,46 @@ info: "See the Technical Writers assigned to Development Guidelines: https://abo **GraphQL at GitLab**: -- [🎬 GitLab Unfiltered GraphQL playlist](https://www.youtube.com/watch?v=wHPKZBDMfxE&list=PL05JrBw4t0KpcjeHjaRMB7IGB2oDWyJzv) -- [🎬 GraphQL at GitLab: Deep Dive](../api_graphql_styleguide.md#deep-dive) (video) by Nick Thomas +<!-- vale gitlab.Spelling = NO --> + +- <i class="fa fa-youtube-play youtube" aria-hidden="true"></i> [GitLab Unfiltered GraphQL playlist](https://www.youtube.com/watch?v=wHPKZBDMfxE&list=PL05JrBw4t0KpcjeHjaRMB7IGB2oDWyJzv) +- <i class="fa fa-youtube-play youtube" aria-hidden="true"></i> [GraphQL at GitLab: Deep Dive](../api_graphql_styleguide.md#deep-dive) (video) by Nick Thomas - An overview of the history of GraphQL at GitLab (not frontend-specific) -- [🎬 GitLab Feature Walkthrough with GraphQL and Vue Apollo](https://www.youtube.com/watch?v=6yYp2zB7FrM) (video) by Natalia Tepluhina +- <i class="fa fa-youtube-play youtube" aria-hidden="true"></i> [GitLab Feature Walkthrough with GraphQL and Vue Apollo](https://www.youtube.com/watch?v=6yYp2zB7FrM) (video) by Natalia Tepluhina - A real-life example of implementing a frontend feature in GitLab using GraphQL -- [🎬 History of client-side GraphQL at GitLab](https://www.youtube.com/watch?v=mCKRJxvMnf0) (video) Illya Klymov and Natalia Tepluhina -- [🎬 From Vuex to Apollo](https://www.youtube.com/watch?v=9knwu87IfU8) (video) by Natalia Tepluhina - - A useful overview of when Apollo might be a better choice than Vuex, and how one could go about the transition +- <i class="fa fa-youtube-play youtube" aria-hidden="true"></i> [History of client-side GraphQL at GitLab](https://www.youtube.com/watch?v=mCKRJxvMnf0) (video) Illya Klymov and Natalia Tepluhina +- <i class="fa fa-youtube-play youtube" aria-hidden="true"></i> [From Vuex to Apollo](https://www.youtube.com/watch?v=9knwu87IfU8) (video) by Natalia Tepluhina + - An overview of when Apollo might be a better choice than Vuex, and how one could go about the transition - [🛠 Vuex -> Apollo Migration: a proof-of-concept project](https://gitlab.com/ntepluhina/vuex-to-apollo/blob/master/README.md) - A collection of examples that show the possible approaches for state management with Vue+GraphQL+(Vuex or Apollo) apps +<!-- vale gitlab.Spelling = YES --> + ### Libraries We use [Apollo](https://www.apollographql.com/) (specifically [Apollo Client](https://www.apollographql.com/docs/react/)) and [Vue Apollo](https://github.com/vuejs/vue-apollo) when using GraphQL for frontend development. -If you are using GraphQL within a Vue application, the [Usage in Vue](#usage-in-vue) section +If you are using GraphQL in a Vue application, the [Usage in Vue](#usage-in-vue) section can help you learn how to integrate Vue Apollo. For other use cases, check out the [Usage outside of Vue](#usage-outside-of-vue) section. +<!-- vale gitlab.Spelling = NO --> + We use [Immer](https://immerjs.github.io/immer/docs/introduction) for immutable cache updates; see [Immutability and cache updates](#immutability-and-cache-updates) for more information. +<!-- vale gitlab.Spelling = YES --> + ### Tooling +<!-- vale gitlab.Spelling = NO --> + - [Apollo Client Devtools](https://github.com/apollographql/apollo-client-devtools) +<!-- vale gitlab.Spelling = YES --> + #### [Apollo GraphQL VS Code extension](https://marketplace.visualstudio.com/items?itemName=apollographql.vscode-apollo) If you use VS Code, the Apollo GraphQL extension supports autocompletion in `.graphql` files. To set up @@ -76,7 +88,7 @@ Our GraphQL API can be explored via GraphiQL at your instance's where needed. You can check all existing queries and mutations on the right side -of GraphiQL in its **Documentation explorer**. It's also possible to +of GraphiQL in its **Documentation explorer**. You can also write queries and mutations directly on the left tab and check their execution by clicking **Execute query** button on the top left: @@ -93,8 +105,8 @@ Default client accepts two parameters: `resolvers` and `config`. - `resolvers` parameter is created to accept an object of resolvers for [local state management](#local-state-with-apollo) queries and mutations - `config` parameter takes an object of configuration settings: - `cacheConfig` field accepts an optional object of settings to [customize Apollo cache](https://www.apollographql.com/docs/react/caching/cache-configuration/#configuring-the-cache) - - `baseUrl` allows us to pass a URL for GraphQL endpoint different from our main endpoint (i.e.`${gon.relative_url_root}/api/graphql`) - - `assumeImmutableResults` (set to `false` by default) - this setting, when set to `true`, will assume that every single operation on updating Apollo Cache is immutable. It also sets `freezeResults` to `true`, so any attempt on mutating Apollo Cache will throw a console warning in development environment. Please ensure you're following the immutability pattern on cache update operations before setting this option to `true`. + - `baseUrl` allows us to pass a URL for GraphQL endpoint different from our main endpoint (for example, `${gon.relative_url_root}/api/graphql`) + - `assumeImmutableResults` (set to `false` by default) - this setting, when set to `true`, assumes that every single operation on updating Apollo Cache is immutable. It also sets `freezeResults` to `true`, so any attempt on mutating Apollo Cache throws a console warning in development environment. Please ensure you're following the immutability pattern on cache update operations before setting this option to `true`. - `fetchPolicy` determines how you want your component to interact with the Apollo cache. Defaults to "cache-first". ## GraphQL Queries @@ -139,7 +151,7 @@ fragment DesignItem on Design { ``` More about fragments: -[GraphQL Docs](https://graphql.org/learn/queries/#fragments) +[GraphQL documentation](https://graphql.org/learn/queries/#fragments) ## Global IDs @@ -157,12 +169,17 @@ const primaryKeyId = getIdFromGraphQLId(data.id); ## Immutability and cache updates -From Apollo version 3.0.0 all the cache updates need to be immutable; it needs to be replaced entirely +From Apollo version 3.0.0 all the cache updates need to be immutable. It needs to be replaced entirely with a **new and updated** object. -To facilitate the process of updating the cache and returning the new object we use the library [Immer](https://immerjs.github.io/immer/docs/introduction). +<!-- vale gitlab.Spelling = NO --> + +To facilitate the process of updating the cache and returning the new object we +use the library [Immer](https://immerjs.github.io/immer/docs/introduction). When possible, follow these conventions: +<!-- vale gitlab.Spelling = YES --> + - The updated cache is named `data`. - The original cache data is named `sourceData`. @@ -184,10 +201,10 @@ client.writeQuery({ ``` As shown in the code example by using `produce`, we can perform any kind of direct manipulation of the -`draftState`. Besides, `immer` guarantees that a new state which includes the changes to `draftState` will be generated. +`draftState`. Besides, `immer` guarantees that a new state which includes the changes to `draftState` is generated. Finally, to verify whether the immutable cache update is working properly, we need to change -`assumeImmutableResults` to `true` in the default client configuration (see [Apollo Client](#apollo-client) for more information). +`assumeImmutableResults` to `true` in the default client configuration. See [Apollo Client](#apollo-client) for more information. If everything is working properly `assumeImmutableResults` should remain set to `true`. @@ -259,11 +276,11 @@ query User { } ``` -Along with creating local data, we can also extend existing GraphQL types with `@client` fields. This is extremely useful when we need to mock an API responses for fields not yet added to our GraphQL API. +Along with creating local data, we can also extend existing GraphQL types with `@client` fields. This is extremely helpful when we need to mock an API response for fields not yet added to our GraphQL API. #### Mocking API response with local Apollo cache -Using local Apollo Cache is handy when we have a need to mock some GraphQL API responses, queries or mutations locally (e.g. when they're still not added to our actual API). +Using local Apollo Cache is helpful when we have a need to mock some GraphQL API responses, queries, or mutations locally (such as when they're still not added to our actual API). For example, we have a [fragment](#fragments) on `DesignVersion` used in our queries: @@ -274,7 +291,7 @@ fragment VersionListItem on DesignVersion { } ``` -We need to fetch also version author and the 'created at' property to display them in the versions dropdown but these changes are still not implemented in our API. We can change the existing fragment to get a mocked response for these new fields: +We also need to fetch the version author and the `created at` property to display in the versions dropdown. But, these changes are still not implemented in our API. We can change the existing fragment to get a mocked response for these new fields: ```javascript fragment VersionListItem on DesignVersion { @@ -288,7 +305,7 @@ fragment VersionListItem on DesignVersion { } ``` -Now Apollo will try to find a _resolver_ for every field marked with `@client` directive. Let's create a resolver for `DesignVersion` type (why `DesignVersion`? because our fragment was created on this type). +Now Apollo tries to find a _resolver_ for every field marked with `@client` directive. Let's create a resolver for `DesignVersion` type (why `DesignVersion`? because our fragment was created on this type). ```javascript // resolvers.js @@ -319,13 +336,13 @@ import resolvers from './graphql/resolvers'; const defaultClient = createDefaultClient(resolvers); ``` -For each attempt to fetch a version, our client will fetch `id` and `sha` from the remote API endpoint and will assign our hardcoded values to the `author` and `createdAt` version properties. With this data, frontend developers are able to work on their UI without being blocked by backend. When the actual response is added to the API, our custom local resolver can be removed and the only change to the query/fragment is to remove the `@client` directive. +For each attempt to fetch a version, our client fetches `id` and `sha` from the remote API endpoint. It then assigns our hardcoded values to the `author` and `createdAt` version properties. With this data, frontend developers are able to work on their UI without being blocked by backend. When the response is added to the API, our custom local resolver can be removed. The only change to the query/fragment is to remove the `@client` directive. Read more about local state management with Apollo in the [Vue Apollo documentation](https://vue-apollo.netlify.app/guide/local-state.html#local-state). ### Using with Vuex -When Apollo Client is used within Vuex and fetched data is stored in the Vuex store, there is no need to keep Apollo Client cache enabled. Otherwise we would have data from the API stored in two places - Vuex store and Apollo Client cache. With Apollo's default settings, a subsequent fetch from the GraphQL API could result in fetching data from Apollo cache (in the case where we have the same query and variables). To prevent this behavior, we need to disable Apollo Client cache by passing a valid `fetchPolicy` option to its constructor: +When the Apollo Client is used in Vuex and fetched data is stored in the Vuex store, the Apollo Client cache does not need to be enabled. Otherwise we would have data from the API stored in two places - Vuex store and Apollo Client cache. With Apollo's default settings, a subsequent fetch from the GraphQL API could result in fetching data from Apollo cache (in the case where we have the same query and variables). To prevent this behavior, we need to disable Apollo Client cache by passing a valid `fetchPolicy` option to its constructor: ```javascript import fetchPolicies from '~/graphql_shared/fetch_policy_constants'; @@ -338,11 +355,61 @@ export const gqClient = createGqClient( ); ``` -### Feature flags in queries +### Working on GraphQL-based features when frontend and backend are not in sync + +Any feature that requires GraphQL queries/mutations to be created or updated should be carefully +planned. Frontend and backend counterparts should agree on a schema that satisfies both client-side and +server-side requirements. This enables both departments to start implementing their parts without +blocking each other. + +Ideally, the backend implementation should be done prior to the frontend so that the client can +immediately start querying the API with minimal back and forth between departments. However, we +recognize that priorities don't always align. For the sake of iteration and +delivering work we're committed to, it might be necessary for the frontend to be implemented ahead +of the backend. + +#### Implementing frontend queries and mutations ahead of the backend -Sometimes it may be useful to have an entity in the GraphQL query behind a feature flag. -For example, when working on a feature where the backend has already been merged but the frontend -hasn't you might want to put the GraphQL entity behind a feature flag to allow for smaller +In such case, the frontend will define GraphQL schemas or fields that do not correspond to any +backend resolver yet. This is fine as long as the implementation is properly feature-flagged so it +does not translate to public-facing errors in the product. However, we do validate client-side +queries/mutations against the backend GraphQL schema with the `graphql-verify` CI job. +You must confirm your changes pass the validation if they are to be merged before the +backend actually supports them. Below are a few suggestions to go about this. + +##### Using the `@client` directive + +The preferred approach is to use the `@client` directive on any new query, mutation, or field that +isn't yet supported by the backend. Any entity with the directive is skipped by the +`graphql-verify` validation job. + +Additionally Apollo will attempt to resolve them client-side, which can be used in conjunction with +[Mocking API response with local Apollo cache](#mocking-api-response-with-local-apollo-cache). This +provides a convenient way of testing your feature with fake data defined client-side. +When opening a merge request for your changes, it can be a good idea to provide local resolvers as a +patch that reviewers can apply in their GDK to easily smoke-test your work. + +Make sure to track the removal of the directive in a follow-up issue, or as part of the backend +implementation plan. + +##### Adding an exception to the list of known failures + +GraphQL queries/mutations validation can be completely turned off for specific files by adding their +paths to the +[`config/known_invalid_graphql_queries.yml`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/config/known_invalid_graphql_queries.yml) +file, much like you would disable ESLint for some files via an `.eslintignore` file. +Bear in mind that any file listed in here will not be validated at all. So if you're only adding +fields to an existing query, use the `@client` directive approach so that the rest of the query +is still validated. + +Again, make sure that those overrides are as short-lived as possible by tracking their removal in +the appropriate issue. + +#### Feature flags in queries + +Sometimes it may be helpful to have an entity in the GraphQL query behind a feature flag. +One example is working on a feature where the backend has already been merged but the frontend +has not. In this case, you may consider putting the GraphQL entity behind a feature flag to allow smaller merge requests to be created and merged. To do this we can use the `@include` directive to exclude an entity if the `if` statement passes. @@ -355,7 +422,7 @@ query getAuthorData($authorNameEnabled: Boolean = false) { ``` Then in the Vue (or JavaScript) call to the query we can pass in our feature flag. This feature -flag will need to be already setup correctly. See the [feature flag documentation](../feature_flags/development.md) +flag needs to be already set up correctly. See the [feature flag documentation](../feature_flags/development.md) for the correct way to do this. ```javascript @@ -469,7 +536,7 @@ Note that we are using the [`pageInfo.fragment.graphql`](https://gitlab.com/gitl #### Using `fetchMore` method in components -This approach makes sense to use with user-handled pagination (e.g. when the scrolls to fetch more data or explicitly clicks a "Next Page"-button). +This approach makes sense to use with user-handled pagination. For example, when the scrolling to fetch more data or explicitly clicking a **Next Page** button. When we need to fetch all the data initially, it is recommended to use [a (non-smart) query, instead](#using-a-recursive-query-in-components). When making an initial fetch, we usually want to start a pagination from the beginning. @@ -479,7 +546,7 @@ In this case, we can either: - Pass `null` explicitly to `after`. After data is fetched, we can use the `update`-hook as an opportunity [to customize -the data that is set in the Vue component property](https://apollo.vuejs.org/api/smart-query.html#options), getting a hold of the `pageInfo` object among other data. +the data that is set in the Vue component property](https://apollo.vuejs.org/api/smart-query.html#options). This allows us to get a hold of the `pageInfo` object among other data. In the `result`-hook, we can inspect the `pageInfo` object to see if we need to fetch the next page. Note that we also keep a `requestCount` to ensure that the application @@ -561,8 +628,8 @@ fetchNextPage(endCursor) { When it is necessary to fetch all paginated data initially an Apollo query can do the trick for us. If we need to fetch the next page based on user interactions, it is recommend to use a [`smartQuery`](https://apollo.vuejs.org/api/smart-query.html) along with the [`fetchMore`-hook](#using-fetchmore-method-in-components). -When the query resolves we can update the component data and inspect the `pageInfo` object -to see if we need to fetch the next page, i.e. call the method recursively. +When the query resolves we can update the component data and inspect the `pageInfo` object. This allows us +to see if we need to fetch the next page, calling the method recursively. Note that we also keep a `requestCount` to ensure that the application does not keep requesting the next page, indefinitely. @@ -634,7 +701,7 @@ or [`.writeQuery()`](https://www.apollographql.com/docs/react/v2/api/apollo-clie This can be tedious and counter-intuitive. To make it easier to deal with cached paginated queries, Apollo provides the `@connection` directive. -The directive accepts a `key` parameter that will be used as a static key when caching the data. +The directive accepts a `key` parameter that is used as a static key when caching the data. You'd then be able to retrieve the data without providing any pagination-specific variables. Here's an example of a query using the `@connection` directive: @@ -661,7 +728,7 @@ query DastSiteProfiles($fullPath: ID!, $after: String, $before: String, $first: } ``` -In this example, Apollo will store the data with the stable `dastSiteProfiles` cache key. +In this example, Apollo stores the data with the stable `dastSiteProfiles` cache key. To retrieve that data from the cache, you'd then only need to provide the `$fullPath` variable, omitting pagination-specific variables like `after` or `before`: @@ -679,9 +746,9 @@ Read more about the `@connection` directive in [Apollo's documentation](https:// ### Managing performance -The Apollo client will batch queries by default. This means that if you have 3 queries defined, -Apollo will group them into one request, send the single request off to the server and only -respond once all 3 queries have completed. +The Apollo client batches queries by default. Given 3 deferred queries, +Apollo groups them into one request, sends the single request to the server, and +responds after all 3 queries have completed. If you need to have queries sent as individual requests, additional context can be provided to tell Apollo to do this. @@ -703,9 +770,13 @@ export default { #### Mocking response as component data -With [Vue test utils](https://vue-test-utils.vuejs.org/) it is easy to quickly test components that +<!-- vale gitlab.Spelling = NO --> + +With [Vue Test Utils](https://vue-test-utils.vuejs.org/) one can quickly test components that fetch GraphQL queries. The simplest way is to use `shallowMount` and then set -the data on the component +the data on the component: + +<!-- vale gitlab.Spelling = YES --> ```javascript it('tests apollo component', () => { @@ -719,7 +790,7 @@ it('tests apollo component', () => { #### Testing loading state -If we need to test how our component renders when results from the GraphQL API are still loading, we can mock a loading state into respective Apollo queries/mutations: +To test how a component renders when results from the GraphQL API are still loading, mock a loading state into respective Apollo queries/mutations: ```javascript function createComponent({ @@ -817,9 +888,9 @@ it('calls mutation on submitting form ', () => { To test the logic of Apollo cache updates, we might want to mock an Apollo Client in our unit tests. We use [`mock-apollo-client`](https://www.npmjs.com/package/mock-apollo-client) library to mock Apollo client and [`createMockApollo` helper](https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/frontend/__helpers__/mock_apollo_helper.js) we created on top of it. -To separate tests with mocked client from 'usual' unit tests, it's recommended to create an additional factory and pass the created `mockApollo` as an option to the `createComponent`-factory. This way we only create Apollo Client instance when it's necessary. +To separate tests with mocked client from 'usual' unit tests, create an additional factory and pass the created `mockApollo` as an option to the `createComponent`-factory. This way we only create Apollo Client instance when it's necessary. -We need to inject `VueApollo` to the Vue local instance and, likewise, it is recommended to call `localVue.use()` within `createMockApolloProvider()` to only load it when it is necessary. +We need to inject `VueApollo` to the Vue local instance and, likewise, it is recommended to call `localVue.use()` in `createMockApolloProvider()` to only load it when it is necessary. ```javascript import VueApollo from 'vue-apollo'; @@ -861,7 +932,7 @@ describe('Some component', () => { }); ``` -Within `createMockApolloProvider`-factory, we need to define an array of _handlers_ for every query or mutation: +In the `createMockApolloProvider`-factory, we need to define an array of _handlers_ for every query or mutation: ```javascript import getDesignListQuery from '~/design_management/graphql/queries/get_design_list.query.graphql'; @@ -1251,9 +1322,9 @@ describe('My Index test with `createMockApollo`', () => { ## Handling errors -The GitLab GraphQL mutations currently have two distinct error modes: [Top-level](#top-level-errors) and [errors-as-data](#errors-as-data). +The GitLab GraphQL mutations have two distinct error modes: [Top-level](#top-level-errors) and [errors-as-data](#errors-as-data). -When utilising a GraphQL mutation, we must consider handling **both of these error modes** to ensure that the user receives the appropriate feedback when an error occurs. +When utilising a GraphQL mutation, consider handling **both of these error modes** to ensure that the user receives the appropriate feedback when an error occurs. ### Top-level errors @@ -1261,13 +1332,13 @@ These errors are located at the "top level" of a GraphQL response. These are non #### Handling top-level errors -Apollo is aware of top-level errors, so we are able to leverage Apollo's various error-handling mechanisms to handle these errors (e.g. handling Promise rejections after invoking the [`mutate`](https://www.apollographql.com/docs/react/api/core/ApolloClient/#ApolloClient.mutate) method, or handling the `error` event emitted from the [`ApolloMutation`](https://apollo.vuejs.org/api/apollo-mutation.html#events) component). +Apollo is aware of top-level errors, so we are able to leverage Apollo's various error-handling mechanisms to handle these errors. For example, handling Promise rejections after invoking the [`mutate`](https://www.apollographql.com/docs/react/api/core/ApolloClient/#ApolloClient.mutate) method, or handling the `error` event emitted from the [`ApolloMutation`](https://apollo.vuejs.org/api/apollo-mutation.html#events) component. Because these errors are not intended for users, error messages for top-level errors should be defined client-side. ### Errors-as-data -These errors are nested within the `data` object of a GraphQL response. These are recoverable errors that, ideally, can be presented directly to the user. +These errors are nested in the `data` object of a GraphQL response. These are recoverable errors that, ideally, can be presented directly to the user. #### Handling errors-as-data @@ -1283,7 +1354,7 @@ mutation createNoteMutation($input: String!) { } ``` -Now, when we commit this mutation and errors occur, the response will include `errors` for us to handle: +Now, when we commit this mutation and errors occur, the response includes `errors` for us to handle: ```javascript { @@ -1316,7 +1387,7 @@ When [using Vuex](#using-with-vuex), disable the cache when: - The data is being cached elsewhere - The use case does not need caching -if the data is being cached elsewhere, or if there is simply no need for it for the given use case. +if the data is being cached elsewhere, or if there is no need for it for the given use case. ```javascript import createDefaultClient from '~/lib/graphql'; @@ -1416,7 +1487,7 @@ for your application. To add GraphQL startup calls, we use `add_page_startup_graphql_call` helper where the first parameter is a path to the query, the second one is an object containing query variables. Path to the query is relative to `app/graphql/queries` folder: for example, if we need a -`app/graphql/queries/repository/files.query.graphql` query, the path will be +`app/graphql/queries/repository/files.query.graphql` query, the path is `repository/files`. ```yaml diff --git a/doc/development/fe_guide/icons.md b/doc/development/fe_guide/icons.md index af587a31bbb..a7b62fbb267 100644 --- a/doc/development/fe_guide/icons.md +++ b/doc/development/fe_guide/icons.md @@ -26,11 +26,11 @@ To use a sprite Icon in HAML or Rails we use a specific helper function: sprite_icon(icon_name, size: nil, css_class: '') ``` -- **icon_name**: Use the icon_name for the SVG sprite in the list of +- **`icon_name`**: Use the `icon_name` for the SVG sprite in the list of ([GitLab SVGs](https://gitlab-org.gitlab.io/gitlab-svgs)). -- **size (optional)**: Use one of the following sizes : 16, 24, 32, 48, 72 (this +- **`size` (optional)**: Use one of the following sizes : 16, 24, 32, 48, 72 (this is translated into a `s16` class) -- **css_class (optional)**: If you want to add additional CSS classes. +- **`css_class` (optional)**: If you want to add additional CSS classes. **Example** @@ -100,7 +100,7 @@ by the examples that follow: - `container` (optional): wraps the loading icon in a container, which centers the loading icon using the `text-center` CSS property. - `color` (optional): either `orange` (default), `light`, or `dark`. - `size` (optional): either `sm` (default), `md`, `lg`, or `xl`. -- `css_class` (optional): defaults to an empty string, but can be useful for utility classes to fine-tune alignment or spacing. +- `css_class` (optional): defaults to an empty string, but can be used for utility classes to fine-tune alignment or spacing. **Example 1:** @@ -164,8 +164,8 @@ export default { ## SVG Illustrations -Please use from now on for any SVG based illustrations simple `img` tags to show an illustration by simply using either `image_tag` or `image_path` helpers. -Please use the class `svg-content` around it to ensure nice rendering. +From now on, use `img` tags to display any SVG based illustrations with either `image_tag` or `image_path` helpers. +Using the class `svg-content` around it ensures nice rendering. ### Usage in HAML/Rails diff --git a/doc/development/fe_guide/index.md b/doc/development/fe_guide/index.md index 84c1623f8c0..711c6a5f875 100644 --- a/doc/development/fe_guide/index.md +++ b/doc/development/fe_guide/index.md @@ -11,8 +11,11 @@ across the GitLab frontend team. ## Overview -GitLab is built on top of [Ruby on Rails](https://rubyonrails.org) using [Haml](https://haml.info/) and also a JavaScript based Frontend with [Vue.js](https://vuejs.org). -Be wary of [the limitations that come with using Hamlit](https://github.com/k0kubun/hamlit/blob/master/REFERENCE.md#limitations). We also use [SCSS](https://sass-lang.com) and plain JavaScript with +GitLab is built on top of [Ruby on Rails](https://rubyonrails.org). It uses [Haml](https://haml.info/) and a JavaScript0based frontend with [Vue.js](https://vuejs.org). +<!-- vale gitlab.Spelling = NO --> +Be wary of [the limitations that come with using Hamlit](https://github.com/k0kubun/hamlit/blob/master/REFERENCE.md#limitations). +<!-- vale gitlab.Spelling = YES --> +We also use [SCSS](https://sass-lang.com) and plain JavaScript with modern ECMAScript standards supported through [Babel](https://babeljs.io/) and ES module support through [webpack](https://webpack.js.org/). Working with our frontend assets requires Node (v10.13.0 or greater) and Yarn @@ -21,7 +24,7 @@ Working with our frontend assets requires Node (v10.13.0 or greater) and Yarn ### Browser Support -For our currently-supported browsers, see our [requirements](../../install/requirements.md#supported-web-browsers). +For supported browsers, see our [requirements](../../install/requirements.md#supported-web-browsers). Use [BrowserStack](https://www.browserstack.com/) to test with our supported browsers. Sign in to BrowserStack with the credentials saved in the **Engineering** vault of the GitLab @@ -56,7 +59,11 @@ Reusable components with technical and usage guidelines can be found in our ## Design Patterns -Common JavaScript [design patterns](design_patterns.md) in the GitLab codebase. +JavaScript [design patterns](design_patterns.md) in the GitLab codebase. + +## Design Anti-patterns + +JavaScript [design anti-patterns](design_anti_patterns.md) we try to avoid. ## Vue.js Best Practices @@ -121,3 +128,7 @@ Our accessibility standards and resources. Frontend internationalization support is described in [this document](../i18n/). The [externalization part of the guide](../i18n/externalization.md) explains the helpers/methods available. + +## [Troubleshooting](troubleshooting.md) + +Running into a Frontend development problem? Check out [this guide](troubleshooting.md) to help resolve your issue. diff --git a/doc/development/fe_guide/performance.md b/doc/development/fe_guide/performance.md index aac2258f3a3..795de87d309 100644 --- a/doc/development/fe_guide/performance.md +++ b/doc/development/fe_guide/performance.md @@ -202,15 +202,15 @@ help identify marks and measures coming from the different apps on the same page ## Best Practices -### Realtime Components +### Real-time Components -When writing code for realtime features we have to keep a couple of things in mind: +When writing code for real-time features we have to keep a couple of things in mind: 1. Do not overload the server with requests. -1. It should feel realtime. +1. It should feel real-time. -Thus, we must strike a balance between sending requests and the feeling of realtime. -Use the following rules when creating realtime solutions. +Thus, we must strike a balance between sending requests and the feeling of real-time. +Use the following rules when creating real-time solutions. 1. The server tells you how much to poll by sending `Poll-Interval` in the header. Use that as your polling interval. This enables system administrators to change the @@ -219,9 +219,9 @@ Use the following rules when creating realtime solutions. 1. A response with HTTP status different from 2XX should disable polling as well. 1. Use a common library for polling. 1. Poll on active tabs only. Please use [Visibility](https://github.com/ai/visibilityjs). -1. Use regular polling intervals, do not use backoff polling, or jitter, as the interval is +1. Use regular polling intervals, do not use <!-- vale gitlab.Spelling = NO --> backoff polling <!-- vale gitlab.Spelling = YES --> or jitter, as the interval is controlled by the server. -1. The backend code is likely to be using etags. You do not and should not check for status +1. The backend code is likely to be using ETags. You do not and should not check for status `304 Not Modified`. The browser transforms it for you. ### Lazy Loading Images @@ -230,12 +230,12 @@ To improve the time to first render we are using lazy loading for images. This w the actual image source on the `data-src` attribute. After the HTML is rendered and JavaScript is loaded, the value of `data-src` is moved to `src` automatically if the image is in the current viewport. -- Prepare images in HTML for lazy loading by renaming the `src` attribute to `data-src` AND adding the class `lazy`. +- Prepare images in HTML for lazy loading by renaming the `src` attribute to `data-src` and adding the class `lazy`. - If you are using the Rails `image_tag` helper, all images are lazy-loaded by default unless `lazy: false` is provided. -If you are asynchronously adding content which contains lazy images then you need to call the function +When asynchronously adding content which contains lazy images, call the function `gl.lazyLoader.searchLazyImages()` which searches for lazy images and loads them if needed. -But in general it should be handled automatically through a `MutationObserver` in the lazy loading function. +In general, it should be handled automatically through a `MutationObserver` in the lazy loading function. ### Animations @@ -243,7 +243,7 @@ Only animate `opacity` & `transform` properties. Other properties (such as `top` Layout to be recalculated, which is much more expensive. For details on this, see "Styles that Affect Layout" in [High Performance Animations](https://www.html5rocks.com/en/tutorials/speed/high-performance-animations/). -If you _do_ need to change layout (for example, a sidebar that pushes main content over), prefer [FLIP](https://aerotwist.com/blog/flip-your-animations/) to change expensive +If you _do_ need to change layout (for example, a sidebar that pushes main content over), prefer [FLIP](https://aerotwist.com/blog/flip-your-animations/). FLIP allows you to change expensive properties once, and handle the actual animation with transforms. ## Reducing Asset Footprint @@ -251,7 +251,7 @@ properties once, and handle the actual animation with transforms. ### Universal code Code that is contained in `main.js` and `commons/index.js` is loaded and -run on _all_ pages. **DO NOT ADD** anything to these files unless it is truly +run on _all_ pages. **Do not add** anything to these files unless it is truly needed _everywhere_. These bundles include ubiquitous libraries like `vue`, `axios`, and `jQuery`, as well as code for the main navigation and sidebar. Where possible we should aim to remove modules from these bundles to reduce our @@ -277,9 +277,9 @@ manually generated webpack bundles. However under this new system you should not ever need to manually add an entry point to the `webpack.config.js` file. NOTE: -If you are unsure what controller and action corresponds to a given page, you -can find this out by inspecting `document.body.dataset.page` in your -browser's developer console while on any page in GitLab. +When unsure what controller and action corresponds to a page, +inspect `document.body.dataset.page` in your +browser's developer console from any page in GitLab. #### Important Considerations @@ -294,7 +294,7 @@ browser's developer console while on any page in GitLab. All GitLab JavaScript files are added with the `defer` attribute. According to the [Mozilla documentation](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script#attr-defer), this implies that "the script is meant to be executed after the document has - been parsed, but before firing `DOMContentLoaded`". Since the document is already + been parsed, but before firing `DOMContentLoaded`". Because the document is already parsed, `DOMContentLoaded` is not needed to bootstrap applications because all the DOM nodes are already at our disposal. @@ -333,7 +333,7 @@ browser's developer console while on any page in GitLab. action(); ``` - For example, see how we use this in [app/assets/javascripts/pages/projects/graphs/charts/index.js](https://gitlab.com/gitlab-org/gitlab/-/commit/5e90885d6afd4497002df55bf015b338efcfc3c5#02e81de37f5b1716a3ef3222fa7f7edf22c40969_9_8): + For example, see how we use this in [`app/assets/javascripts/pages/projects/graphs/charts/index.js`](https://gitlab.com/gitlab-org/gitlab/-/commit/5e90885d6afd4497002df55bf015b338efcfc3c5#02e81de37f5b1716a3ef3222fa7f7edf22c40969_9_8): ```javascript waitForCSSLoaded(() => { @@ -366,9 +366,9 @@ browser's developer console while on any page in GitLab. ### Code Splitting -For any code that does not need to be run immediately upon page load, (for example, -modals, dropdowns, and other behaviors that can be lazy-loaded), you can split -your module into asynchronous chunks with dynamic import statements. These +Code that does not need to be run immediately upon page load (for example, +modals, dropdowns, and other behaviors that can be lazy-loaded) should be split +into asynchronous chunks with dynamic import statements. These imports return a Promise which is resolved after the script has loaded: ```javascript @@ -377,16 +377,16 @@ import(/* webpackChunkName: 'emoji' */ '~/emoji') .catch(/* report error */) ``` -Please try to use `webpackChunkName` when generating these dynamic imports as +Use `webpackChunkName` when generating dynamic imports as it provides a deterministic filename for the chunk which can then be cached -the browser across GitLab versions. +in the browser across GitLab versions. More information is available in [webpack's code splitting documentation](https://webpack.js.org/guides/code-splitting/#dynamic-imports). ### 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 +A smaller page size means the page loads faster, especially 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: diff --git a/doc/development/fe_guide/security.md b/doc/development/fe_guide/security.md index 627c5f4d12f..df4613d521d 100644 --- a/doc/development/fe_guide/security.md +++ b/doc/development/fe_guide/security.md @@ -8,7 +8,7 @@ info: To determine the technical writer assigned to the Stage/Group associated w ## Resources -[Mozilla’s HTTP Observatory CLI](https://github.com/mozilla/http-observatory-cli) and the +[Mozilla’s HTTP Observatory CLI](https://github.com/mozilla/http-observatory-cli) and [Qualys SSL Labs Server Test](https://www.ssllabs.com/ssltest/analyze.html) are good resources for finding potential problems and ensuring compliance with security best practices. @@ -76,7 +76,7 @@ such as with reCAPTCHA, which cannot be used without an `iframe`. In order to protect users from [XSS vulnerabilities](https://en.wikipedia.org/wiki/Cross-site_scripting), we intend to disable inline scripts in the future using Content Security Policy. -While inline scripts can be useful, they're also a security concern. If +While inline scripts can make something easier, they're also a security concern. If user-supplied content is unintentionally left un-sanitized, malicious users can inject scripts into the web app. diff --git a/doc/development/fe_guide/style/html.md b/doc/development/fe_guide/style/html.md index 7fedbc6ce0d..e53686de1a0 100644 --- a/doc/development/fe_guide/style/html.md +++ b/doc/development/fe_guide/style/html.md @@ -6,6 +6,38 @@ info: To determine the technical writer assigned to the Stage/Group associated w # HTML style guide +## Semantic elements + +[Semantic elements](https://developer.mozilla.org/en-US/docs/Glossary/Semantics) are HTML tags that +give semantic (rather than presentational) meaning to the data they contain. For example: + +- [`<article>`](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/article) +- [`<nav>`](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/nav) +- [`<strong>`](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/strong) + +Prefer using semantic tags, but only if the intention is truly accurate with the semantic meaning +of the tag itself. View the [MDN documentation](https://developer.mozilla.org/en-US/docs/Web/HTML/Element) +for a description on what each tag semantically means. + +```html +<!-- bad - could use semantic tags instead of div's. --> +<div class="..."> + <p> + <!-- bad - this isn't what "strong" is meant for. --> + Simply visit your <strong>Settings</strong> to say hello to the world. + </p> + <div class="...">...</div> +</div> + +<!-- good - prefer semantic classes used accurately --> +<section class="..."> + <p> + Simply visit your <span class="gl-font-weight-bold">Settings</span> to say hello to the world. + </p> + <footer class="...">...</footer> +</section> +``` + ## Buttons ### Button type diff --git a/doc/development/fe_guide/style/javascript.md b/doc/development/fe_guide/style/javascript.md index faf03a03101..5c35b880eab 100644 --- a/doc/development/fe_guide/style/javascript.md +++ b/doc/development/fe_guide/style/javascript.md @@ -14,7 +14,7 @@ In addition to the style guidelines set by Airbnb, we also have a few specific r listed below. NOTE: -You can run eslint locally by running `yarn eslint` +You can run ESLint locally by running `yarn eslint` ## Avoid forEach diff --git a/doc/development/fe_guide/style/scss.md b/doc/development/fe_guide/style/scss.md index a4cae12c4f3..1d0b3c2b7fc 100644 --- a/doc/development/fe_guide/style/scss.md +++ b/doc/development/fe_guide/style/scss.md @@ -12,7 +12,7 @@ easy to maintain, and performant for the end-user. ## Rules -Our CSS is a mixture of current and legacy approaches. That means sometimes it may be difficult to follow this guide to the letter; it means you are likely to run into exceptions, where following the guide is difficult to impossible without outsized effort. In those cases, you may work with your reviewers and maintainers to identify an approach that does not fit these rules. Please endeavor to limit these cases. +Our CSS is a mixture of current and legacy approaches. That means sometimes it may be difficult to follow this guide to the letter; it means you are likely to run into exceptions, where following the guide is difficult to impossible without major effort. In those cases, you may work with your reviewers and maintainers to identify an approach that does not fit these rules. Please endeavor to limit these cases. ### Utility Classes @@ -20,15 +20,19 @@ In order to reduce the generation of more CSS as our site grows, prefer the use #### Where are utility classes defined? -Prefer the use of [utility classes defined in GitLab UI](https://gitlab.com/gitlab-org/gitlab-ui/-/blob/master/doc/css.md#utilities). An easy list of classes can also be [seen on Unpkg](https://unpkg.com/browse/@gitlab/ui/src/scss/utilities.scss). +Prefer the use of [utility classes defined in GitLab UI](https://gitlab.com/gitlab-org/gitlab-ui/-/blob/main/doc/css.md#utilities). +<!-- vale gitlab.Spelling = NO --> +An easy list of classes can also be [seen on Unpkg](https://unpkg.com/browse/@gitlab/ui/src/scss/utilities.scss). +<!-- vale gitlab.Spelling = YES --> -Classes in [`utilities.scss`](https://gitlab.com/gitlab-org/gitlab/blob/master/app/assets/stylesheets/utilities.scss) and [`common.scss`](https://gitlab.com/gitlab-org/gitlab/blob/master/app/assets/stylesheets/framework/common.scss) are being deprecated. Classes in [`common.scss`](https://gitlab.com/gitlab-org/gitlab/blob/master/app/assets/stylesheets/framework/common.scss) that use non-design system values should be avoided in favor of conformant values. +Classes in [`utilities.scss`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/assets/stylesheets/utilities.scss) and [`common.scss`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/assets/stylesheets/framework/common.scss) are being deprecated. +Classes in [`common.scss`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/assets/stylesheets/framework/common.scss) that use non-design-system values should be avoided. Use classes with conforming values instead. Avoid [Bootstrap's Utility Classes](https://getbootstrap.com/docs/4.3/utilities/). NOTE: While migrating [Bootstrap's Utility Classes](https://getbootstrap.com/docs/4.3/utilities/) -to the [GitLab UI](https://gitlab.com/gitlab-org/gitlab-ui/-/blob/master/doc/css.md#utilities) +to the [GitLab UI](https://gitlab.com/gitlab-org/gitlab-ui/-/blob/main/doc/css.md#utilities) utility classes, note both the classes for margin and padding differ. The size scale used at GitLab differs from the scale used in the Bootstrap library. For a Bootstrap padding or margin utility, you may need to double the size of the applied utility to achieve the same visual @@ -36,9 +40,9 @@ result (such as `ml-1` becoming `gl-ml-2`). #### Where should I put new utility classes? -If a class you need has not been added to GitLab UI, you get to add it! Follow the naming patterns documented in the [utility files](https://gitlab.com/gitlab-org/gitlab-ui/-/tree/master/src/scss/utility-mixins) and refer to [GitLab UI's CSS documentation](https://gitlab.com/gitlab-org/gitlab-ui/-/blob/master/doc/contributing/adding_css.md#adding-utility-mixins) for more details, especially about adding responsive and stateful rules. +If a class you need has not been added to GitLab UI, you get to add it! Follow the naming patterns documented in the [utility files](https://gitlab.com/gitlab-org/gitlab-ui/-/tree/main/src/scss/utility-mixins) and refer to [GitLab UI's CSS documentation](https://gitlab.com/gitlab-org/gitlab-ui/-/blob/main/doc/contributing/adding_css.md#adding-utility-mixins) for more details, especially about adding responsive and stateful rules. -If it is not possible to wait for a GitLab UI update (generally one day), add the class to [`utilities.scss`](https://gitlab.com/gitlab-org/gitlab/blob/master/app/assets/stylesheets/utilities.scss) following the same naming conventions documented in GitLab UI. A follow—up issue to backport the class to GitLab UI and delete it from GitLab should be opened. +If it is not possible to wait for a GitLab UI update (generally one day), add the class to [`utilities.scss`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/assets/stylesheets/utilities.scss) following the same naming conventions documented in GitLab UI. A follow—up issue to backport the class to GitLab UI and delete it from GitLab should be opened. #### When should I create component classes? @@ -47,7 +51,7 @@ 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`). +This encourages an organic growth of component classes and prevents the creation of one-off non-reusable 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`). Inspiration: @@ -143,7 +147,7 @@ documentation includes [a full list of their linters](https://github.com/sds/scs If you want to automate changing a large portion of the codebase to conform to the SCSS style guide, you can use [CSSComb](https://github.com/csscomb/csscomb.js). First install -[Node](https://github.com/nodejs/node) and [NPM](https://www.npmjs.com/), then run `npm install csscomb -g` to install +[Node](https://github.com/nodejs/node) and [npm](https://www.npmjs.com/), 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. diff --git a/doc/development/fe_guide/style_guide_js.md b/doc/development/fe_guide/style_guide_js.md deleted file mode 100644 index 73a5bea189d..00000000000 --- a/doc/development/fe_guide/style_guide_js.md +++ /dev/null @@ -1,8 +0,0 @@ ---- -redirect_to: 'style/javascript.md' ---- - -This document was moved to [another location](style/javascript.md). - -<!-- This redirect file can be deleted after February 1, 2021. --> -<!-- Before deletion, see: https://docs.gitlab.com/ee/development/documentation/#move-or-rename-a-page --> diff --git a/doc/development/fe_guide/style_guide_scss.md b/doc/development/fe_guide/style_guide_scss.md deleted file mode 100644 index eaa6d33fb43..00000000000 --- a/doc/development/fe_guide/style_guide_scss.md +++ /dev/null @@ -1,8 +0,0 @@ ---- -redirect_to: 'style/scss.md' ---- - -This document was moved to [another location](style/scss.md). - -<!-- This redirect file can be deleted after February 1, 2021. --> -<!-- Before deletion, see: https://docs.gitlab.com/ee/development/documentation/#move-or-rename-a-page --> diff --git a/doc/development/fe_guide/testing.md b/doc/development/fe_guide/testing.md deleted file mode 100644 index 457d15235ae..00000000000 --- a/doc/development/fe_guide/testing.md +++ /dev/null @@ -1,8 +0,0 @@ ---- -redirect_to: '../testing_guide/frontend_testing.md' ---- - -This document was moved to [another location](../testing_guide/frontend_testing.md). - -<!-- This redirect file can be deleted after February 1, 2021. --> -<!-- Before deletion, see: https://docs.gitlab.com/ee/development/documentation/#move-or-rename-a-page --> diff --git a/doc/development/fe_guide/tooling.md b/doc/development/fe_guide/tooling.md index d33022b9355..7a2d8fccdbf 100644 --- a/doc/development/fe_guide/tooling.md +++ b/doc/development/fe_guide/tooling.md @@ -111,7 +111,7 @@ preferred editor (all major editors are supported) accordingly. We suggest setting up Prettier to run when each file is saved. For instructions about using Prettier in your preferred editor, see the [Prettier documentation](https://prettier.io/docs/en/editors.html). -Please take care that you only let Prettier format the same file types as the global Yarn script does (`.js`, `.vue`, `.graphql`, and `.scss`). In VSCode by example you can easily exclude file formats in your settings file: +Please take care that you only let Prettier format the same file types as the global Yarn script does (`.js`, `.vue`, `.graphql`, and `.scss`). For example, you can exclude file formats in your Visual Studio Code settings file: ```json "prettier.disableLanguages": [ @@ -128,13 +128,13 @@ 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. +Updates all staged files (based on `git diff`) with Prettier and saves the needed changes. ```shell 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. +Checks all staged files (based on `git diff`) with Prettier and log which files would need manual updating to the console. ```shell yarn prettier-all diff --git a/doc/development/fe_guide/troubleshooting.md b/doc/development/fe_guide/troubleshooting.md new file mode 100644 index 00000000000..abaf9cd68c7 --- /dev/null +++ b/doc/development/fe_guide/troubleshooting.md @@ -0,0 +1,41 @@ +--- +stage: none +group: unassigned +info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/engineering/ux/technical-writing/#assignments +--- + +# Troubleshooting + +Running into a problem? Maybe this will help ¯\_(ツ)_/¯. + +## Troubleshooting issues + +### This guide doesn't contain the issue I ran into + +If you run into a Frontend development issue that is not in this guide, please consider updating this guide with your issue and possible remedies. This way future adventurers can face these dragons with more success, being armed with your experience and knowledge. + +## Testing issues + +### ``Property or method `nodeType` is not defined`` but I'm not using `nodeType` anywhere + +This issue can happen in Vue component tests, when an expectation fails, but there is an error thrown when +Jest tries to pretty print the diff in the console. It's been noted that using `toEqual` with an array as a +property might also be a contributing factor. + +See [this video](https://youtu.be/-BkEhghP-kM) for an in-depth overview and investigation. + +**Remedy - Try cloning the object that has Vue watchers** + +```patch +- expect(wrapper.find(ChildComponent).props()).toEqual(...); ++ expect(cloneDeep(wrapper.find(ChildComponent).props())).toEqual(...) +``` + +**Remedy - Try using `toMatchObject` instead of `toEqual`** + +```patch +- expect(wrapper.find(ChildComponent).props()).toEqual(...); ++ expect(wrapper.find(ChildComponent).props()).toMatchObject(...); +``` + +Please note that `toMatchObject` actually changes the nature of the assertion and won't fail if some items are **missing** from the expectation. diff --git a/doc/development/fe_guide/vue.md b/doc/development/fe_guide/vue.md index b3fbb9556a9..5b902e1b16e 100644 --- a/doc/development/fe_guide/vue.md +++ b/doc/development/fe_guide/vue.md @@ -20,9 +20,9 @@ What is described in the following sections can be found in these examples: All new features built with Vue.js must follow a [Flux architecture](https://facebook.github.io/flux/). The main goal we are trying to achieve is to have only one data flow and only one data entry. -In order to achieve this goal we use [vuex](#vuex). +In order to achieve this goal we use [Vuex](#vuex). -You can also read about this architecture in Vue docs about +You can also read about this architecture in Vue documentation about [state management](https://vuejs.org/v2/guide/state-management.html#Simple-State-Management-from-Scratch) and about [one way data flow](https://vuejs.org/v2/guide/components.html#One-Way-Data-Flow). @@ -70,7 +70,7 @@ The advantage of providing data from the DOM to the Vue instance through `props` function instead of querying the DOM inside the main Vue component is avoiding the need to create a fixture or an HTML element in the unit test, which makes the tests easier. -See the following example, also, please refer to our [Vue style guide](style/vue.md#basic-rules) for +See the following example. Also, please refer to our [Vue style guide](style/vue.md#basic-rules) for additional information on why we explicitly declare the data being passed into the Vue app; ```javascript @@ -101,8 +101,8 @@ across the codebase. #### Accessing the `gl` object -When we need to query the `gl` object for data that doesn't change during the application's life -cycle, we should do it in the same place where we query the DOM. By following this practice, we can +We query the `gl` object for data that doesn't change during the application's life +cycle in the same place we query the DOM. By following this practice, we can avoid the need to mock the `gl` object, which makes tests easier. It should be done while initializing our Vue instance, and the data should be provided as `props` to the main component: @@ -148,8 +148,8 @@ This approach has a few benefits: - Arbitrarily deeply nested components can opt-in and access the flag without intermediate components being aware of it (c.f. passing the flag down via props). -- Good testability, since the flag can be provided to `mount`/`shallowMount` - from `vue-test-utils` simply as a prop. +- Good testability, because the flag can be provided to `mount`/`shallowMount` + from `vue-test-utils` as a prop. ```javascript import { shallowMount } from '@vue/test-utils'; @@ -207,20 +207,20 @@ Based on the Vue guidance: such as `user: new User()`. - **Do not** add new JavaScript class implementations. - **Do** use [GraphQL](../api_graphql_styleguide.md), [Vuex](vuex.md) or a set of components if -cannot use simple primitives or objects. +cannot use primitives or objects. - **Do** maintain existing implementations using such approaches. - **Do** Migrate components to a pure object model when there are substantial changes to it. -- **Do** add business logic to helpers or utils, so you can test them separately from your component. +- **Do** add business logic to helpers or utilities, so you can test them separately from your component. #### Why There are additional reasons why having a JavaScript class presents maintainability issues on a huge codebase: -- Once a class is created, it is easy to extend it in a way that can infringe Vue reactivity and best practices. +- After a class is created, it can be extended in a way that can infringe Vue reactivity and best practices. - A class adds a layer of abstraction, which makes the component API and its inner workings less clear. -- It makes it harder to test. Since the class is instantiated by the component data function, it is +- It makes it harder to test. Because the class is instantiated by the component data function, it is harder to 'manage' component and class separately. -- Adding OOP to a functional codebase adds yet another way of writing code, reducing consistency and clarity. +- Adding Object Oriented Principles (OOP) to a functional codebase adds yet another way of writing code, reducing consistency and clarity. ## Style guide @@ -234,8 +234,8 @@ for guidelines and best practices for testing your Vue components. Each Vue component has a unique output. This output is always present in the render function. -Although we can test each method of a Vue component individually, our goal must be to test the output -of the render/template function, which represents the state at all times. +Although each method of a Vue component can be tested individually, our goal is to test the output +of the render function, which represents the state at all times. Here's an example of a well structured unit test for [this Vue component](#appendix---vue-component-subject-under-test): @@ -345,12 +345,12 @@ component under test, with the `computed` property, for example). Remember to us ```javascript const checkbox = wrapper.findByTestId('checkboxTestId'); - + expect(checkbox.attributes('disabled')).not.toBeDefined(); - + findChildComponent().vm.$emit('primary'); await nextTick(); - + expect(checkbox.attributes('disabled')).toBeDefined(); ``` @@ -359,14 +359,14 @@ component under test, with the `computed` property, for example). Remember to us ```javascript // bad expect(findChildComponent().find('.error-alert').exists()).toBe(false); - + // good expect(findChildComponent().props('withAlertContainer')).toBe(false); ``` ### Events -We should test for events emitted in response to an action within our component, this is useful to +We should test for events emitted in response to an action in our component. This is used to verify the correct events are being fired with the correct arguments. For any DOM events we should use [`trigger`](https://vue-test-utils.vuejs.org/api/wrapper/#trigger) @@ -416,7 +416,7 @@ You should only apply to be a Vue.js expert when your own merge requests and you > This section is added temporarily to support the efforts to migrate the codebase from Vue 2.x to Vue 3.x -Currently, we recommend to minimize adding certain features to the codebase to prevent increasing +We recommend to minimize adding certain features to the codebase to prevent increasing the tech debt for the eventual migration: - filters; diff --git a/doc/development/fe_guide/vue3_migration.md b/doc/development/fe_guide/vue3_migration.md index 9d2f3b27968..ee25e97ab6e 100644 --- a/doc/development/fe_guide/vue3_migration.md +++ b/doc/development/fe_guide/vue3_migration.md @@ -26,7 +26,7 @@ Component's computed properties / methods or external helpers. **What to use instead** -Vue docs recommend using [mitt](https://github.com/developit/mitt) library. It's relatively small (200 bytes gzipped) and has a simple API: +Vue documentation recommends using the [mitt](https://github.com/developit/mitt) library. It's relatively small (200 bytes, compressed) and has a clear API: ```javascript import mitt from 'mitt' @@ -51,9 +51,9 @@ emitter.off('foo', onFoo) // unlisten **Event hub factory** -To make it easier for you to migrate existing event hubs to the new recommended approach, or simply -to create new ones, we have created a factory that you can use to instantiate a new mitt-based -event hub. +We have created a factory that you can use to instantiate a new mitt-based event hub. +This makes it easier to migrate existing event hubs to the new recommended approach, or +to create new ones. ```javascript import createEventHub from '~/helpers/event_hub_factory'; @@ -88,7 +88,7 @@ It is not recommended to replace stateful components with functional components **Why?** -In Vue 2.6 `slot` attribute was already deprecated in favor of `v-slot` directive but its usage is still allowed and sometimes we prefer using them because it simplifies unit tests (with old syntax, slots are rendered on `shallowMount`). However, in Vue 3 we can't use old syntax anymore. +In Vue 2.6 `slot` attribute was already deprecated in favor of `v-slot` directive. The `slot` attribute usage is still allowed and sometimes we prefer using it because it simplifies unit tests (with old syntax, slots are rendered on `shallowMount`). However, in Vue 3 we can't use old syntax anymore. **What to use instead** diff --git a/doc/development/fe_guide/vuex.md b/doc/development/fe_guide/vuex.md index 1d83335291a..cc1d9ccab77 100644 --- a/doc/development/fe_guide/vuex.md +++ b/doc/development/fe_guide/vuex.md @@ -146,7 +146,7 @@ The only way to change state in a Vuex store is by committing a mutation. Most mutations are committed from an action using `commit`. If you don't have any asynchronous operations, you can call mutations from a component using the `mapMutations` helper. -See the Vuex docs for examples of [committing mutations from components](https://vuex.vuejs.org/guide/mutations.html#committing-mutations-in-components). +See the Vuex documentation for examples of [committing mutations from components](https://vuex.vuejs.org/guide/mutations.html#committing-mutations-in-components). #### Naming Pattern: `REQUEST` and `RECEIVE` namespaces @@ -271,7 +271,7 @@ import { mapGetters } from 'vuex'; ### `mutation_types.js` -From [vuex mutations docs](https://vuex.vuejs.org/guide/mutations.html): +From [Vuex mutations documentation](https://vuex.vuejs.org/guide/mutations.html): > It is a commonly seen pattern to use constants for mutation types in various Flux implementations. > This allows the code to take advantage of tooling like linters, and putting all constants in a > single file allows your collaborators to get an at-a-glance view of what mutations are possible @@ -429,7 +429,7 @@ export default { #### Testing Vuex concerns -Refer to [Vuex docs](https://vuex.vuejs.org/guide/testing.html) regarding testing Actions, Getters and Mutations. +Refer to [Vuex documentation](https://vuex.vuejs.org/guide/testing.html) regarding testing Actions, Getters and Mutations. #### Testing components that need a store |