# Code review as a link

Writing comments in Merge Requests? Write it here instead and post a link in GitLab. This way over time we build a comprehensive documentation of conventions and best practices we use.

The only exception when you should not write a comment here is when the comment is specific to a particular situation and you don't think it would be useful in other contexts.

:::info
Don't change headers' text or it will break the link to it.
:::

# Styling

## Don't construct class names dynamically

// bad
<div class="text-{{ error ? 'red' : 'green' }}-600"></div>

// good
<div class="{{ error ? 'text-red-600' : 'text-green-600' }}"></div>

See [class detection in depth](https://tailwindcss.com/docs/content-configuration#class-detection-in-depth)
Dynamic class names also break [the grep test](http://jamie-wong.com/2013/07/12/grep-test/).

## Capital letters

Always capitalize with css unless the text is an acronym.

That's because screen readers will read capitalized words as separate letters, not full words.

// BAD (see what I did here? 😜)
<span>VOTING</span>
<span className="uppercase">gps</span>

// Good
<span className="uppercase">voting</span>
<span>GPS</span>

See [Capital Letters and Accessibility](https://freshysites.com/wordpress/ada-compliance/capital-letters-accessibility/).

# Merge Requests

## The size of Merge Requests

Please keep your merge requests as small as possible, preferably under 500 LOC (lines of code).

Anything beyond that is hard to review.

Generally, the bigger the Merge Request, then longer it takes to review and improve.

# Design patterns and widgets
All widgets should follow [WAI-ARIA Authoring Practices](https://www.w3.org/TR/wai-aria-practices/#aria_ex).

In practice, there is lots of requirements to seemingly simple widgets like a modal.

For this reason, avoid implementing them yourself.

Instead, use a library like [Reach UI](https://reach.tech/).

# TypeScript
## Avoid Cluttering Your Code with Inferable Types

// bad
const x: number = 12;

// good
const x = 12;

See https://effectivetypescript.com/2020/04/28/avoid-inferable/

# State
## Avoid derived state
See https://benmccormick.org/2019/06/24/derived-state.

# Naming

## Readability
Alwyas prefer descripitve and meaningful over generic and meaningless names.

Prioritize readability over brevity.

// bad
const connected = true

// good
const isWalletConnected = true

## Props Interfaces

Always name your props interface after component name.

This will simplify the job if we decide to export that interface.

// bad
interface Props {}

// good
interface ButtonProps {}

See https://en.wikipedia.org/wiki/Naming_convention_(programming)#Readability

# Env variables

## When to use env variables?
- API keys
- URLs to external services
- Anything we would like to update without redeploying the app

# Global objects
Avoid modyfing global objects like `window`. Any other piece of code can potentially modify a global object and break your code. See [why to avoid global variables in JavaScript](https://www.geeksforgeeks.org/why-to-avoid-global-variables-in-javascript/).

# svg
## .tsx or .svg?
If you can, avoid rendering svg in jsx. Instead, import `.svg` files directly.

// Instead of doing this:
import FancyIcon from 'fancy-icon.tsx'
return <FancyIcon />

// do this:
import fancyIconUrl from 'fancy-icon.svg'
return <img src={fancyIconUrl} />

There are many reasons for this:

1. images are cached for longer on the CDN
1. images can be optimized with tools like svgo
1. images avoid clashing ids problems

A rare exception is when you want to dynamically change a property of part of an svg, eg. change the color of part of svg element on hover. Then you're better off rendering svg elements in jsx.