Document quo best practices and guidelines (#16901)
This commit is contained in:
Icaro Motta
GitHub
parent
528df179d9
commit
6550bc058d
Binary file not shown.
|
After Width: | Height: | Size: 72 KiB |
Binary file not shown.
|
After Width: | Height: | Size: 64 KiB |
|
|
@ -1,8 +1,9 @@
|
|||
# Code Style Guidelines
|
||||
|
||||
>The goal of this document is to help all contributors (core and external) to
|
||||
>write code in _unison_ and help establish good practices that serve the Status
|
||||
>Mobile contributors well.
|
||||
> [!IMPORTANT]
|
||||
> The goal of this document is to help all contributors (core and external) to
|
||||
> write code in _unison_ and help establish good practices that serve the Status
|
||||
> Mobile contributors well.
|
||||
|
||||
We don't want to turn this document into an exhaustive list of rules to be
|
||||
followed that nobody will read. As much as possible, we'll try to document only
|
||||
|
|
@ -10,35 +11,28 @@ what we consider important for Status Mobile. In other words, we don't want to
|
|||
maintain a general Clojure convention/style guide, nor do we want to turn this
|
||||
document into a long tutorial.
|
||||
|
||||
This is a **work in progress**, and not all conventions are properly implemented
|
||||
in the codebase yet. The project structure is also going over major changes (as
|
||||
of Nov/2022), and it'll take a considerable amount of time until we migrate the
|
||||
existing code to the new structure.
|
||||
> [!WARNING]
|
||||
> This is a **work in progress**, and not all conventions are properly
|
||||
> implemented in the codebase yet. The project structure is also undergoing
|
||||
> major changes, and it will take a considerable amount of time until we migrate
|
||||
> the existing code to the new structure.
|
||||
|
||||
If you find out anything is outdated or missing, please, share with us or even
|
||||
better, create a pull-request! 🤸
|
||||
|
||||
## Style guide
|
||||
|
||||
We mostly follow the [Clojure Style
|
||||
Guide](https://github.com/bbatsov/clojure-style-guide), so it's recommended to
|
||||
get familiar with it.
|
||||
|
||||
As of Nov/2022, running `make lint` should fix the most basic formatting issues,
|
||||
and we are in the process of integrating a tool to format non-trivial code
|
||||
according to the Clojure Style Guide. This should greatly reduce noise in
|
||||
pull-request reviews and it'll simplify the life of all contributors.
|
||||
|
||||
Pay special attention to:
|
||||
|
||||
- Align let bindings https://github.com/bbatsov/clojure-style-guide#bindings-alignment
|
||||
- Align map keys https://github.com/bbatsov/clojure-style-guide#map-keys-alignment
|
||||
We follow the [Clojure Style
|
||||
Guide](https://github.com/bbatsov/clojure-style-guide) and we use
|
||||
[zprint](https://github.com/kkinnear/zprint) to format Clojure code. Running
|
||||
`make lint-fix` should fix most formatting issues, but not all of them.
|
||||
|
||||
## Dos and don'ts
|
||||
|
||||
### Hiccup
|
||||
|
||||
Never use anonymous inline function in hiccup, this will lead to reinitialization of component on each render of parent component
|
||||
Never use anonymous inline function in hiccup, this will lead to
|
||||
reinitialization of component on each render of parent component.
|
||||
|
||||
```clojure
|
||||
;; bad
|
||||
|
|
@ -57,7 +51,7 @@ Never use anonymous inline function in hiccup, this will lead to reinitializatio
|
|||
[comp]])
|
||||
```
|
||||
|
||||
This mistake mostly happens with functional components
|
||||
This mistake mostly happens with functional components.
|
||||
|
||||
```clojure
|
||||
;; bad
|
||||
|
|
@ -71,20 +65,24 @@ This mistake mostly happens with functional components
|
|||
;; good
|
||||
(defn f-comp [atom]
|
||||
[rn/text atom])
|
||||
|
||||
|
||||
(fn []
|
||||
(let [atom (rf/sub [:sub])]
|
||||
(fn []
|
||||
[:f> f-comp atom])))
|
||||
[:f> f-comp atom])))
|
||||
```
|
||||
|
||||
it's important to name functional components with `f-` prefix
|
||||
It's important to name functional components with `f-` prefix.
|
||||
|
||||
### Component props and API scheme to match Figma as closely as possible
|
||||
|
||||
Ideally, the prop names for components (particularly in quo2 Design System) should match the Figma properties as best as possible. This makes it easier for the developer using that component to configure it correctly for the screen it is being used on and avoids unnecessary overwrites and adjustments being made.
|
||||
Ideally, the prop names for components (particularly in quo2 Design System)
|
||||
should match the Figma properties as best as possible. This makes it easier for
|
||||
the developer using that component to configure it correctly for the screen it
|
||||
is being used on and avoids unnecessary overwrites and adjustments being made.
|
||||
|
||||
#### Avoid unnecessarily grouping categories to reduce the number of props
|
||||
|
||||
For example in Figma if there is a component and it has the following variants:
|
||||
|
||||
|theme: "light" blur: "False"|theme: "dark" blur: "False"|theme: "light" blur: "True"|theme: "dark" blur: "True"|
|
||||
|
|
@ -107,12 +105,18 @@ type - can be :neutral :active :danger :danger-blur"
|
|||
(defn my-component [{:keys [theme blur? type]} label])
|
||||
```
|
||||
|
||||
Please note this is only for the external API of the component and there should be no restriction of how the component manages its internal API as that will not affect the developer using the component with the issues described above.
|
||||
Please note this is only for the external API of the component and there should
|
||||
be no restriction of how the component manages its internal API as that will not
|
||||
affect the developer using the component with the issues described above.
|
||||
|
||||
In some cases this is not always possible or does not make sense. However the thought process should be how easy will it be for another developer to use this component with the correct configuration given the screen designs for Figma.
|
||||
In some cases this is not always possible or does not make sense. However the
|
||||
thought process should be how easy will it be for another developer to use this
|
||||
component with the correct configuration given the screen designs for Figma.
|
||||
|
||||
#### Avoid unnecessarily renaming props
|
||||
In general it can be helpful to avoid renaming props from their counterpart in Figma.
|
||||
#### Avoid unnecessarily renaming props
|
||||
|
||||
In general it can be helpful to avoid renaming props from their counterpart in
|
||||
Figma.
|
||||
|
||||
For example if Figma has sizes `:small`, `:medium` and `:large`
|
||||
|
||||
|
|
@ -153,7 +157,7 @@ the source file. For a real example, see
|
|||
|
||||
### Always add styles inside the `:style` key
|
||||
|
||||
Although when compiling ReactNative for mobile some components are able work with
|
||||
Although when compiling ReactNative for mobile some components are able work with
|
||||
their styles in the top-level of the properties map, prefer to add them inside the
|
||||
`:style` key in order to separate styles from properties:
|
||||
|
||||
|
|
@ -172,12 +176,11 @@ their styles in the top-level of the properties map, prefer to add them inside t
|
|||
:on-press #(js/alert "Hi!")
|
||||
:title "Button"}]
|
||||
|
||||
;; better
|
||||
;; better
|
||||
;; (define them in a style ns & place them inside `:style` key)
|
||||
[rn/button {:style (style/button)
|
||||
:on-press #(js/alert "Hi!")
|
||||
:title "Button"}
|
||||
]
|
||||
:title "Button"}]
|
||||
```
|
||||
|
||||
Also its fine to keep one liner styles in view
|
||||
|
|
@ -189,7 +192,7 @@ Also its fine to keep one liner styles in view
|
|||
|
||||
### Don't define properties in styles ns
|
||||
|
||||
Properties must be set on view level
|
||||
Properties must be set on view level
|
||||
|
||||
```clojure
|
||||
;; bad
|
||||
|
|
@ -209,10 +212,8 @@ Properties must be set on view level
|
|||
:bottom 0}
|
||||
```
|
||||
|
||||
|
||||
### Apply animated styles in the style file
|
||||
|
||||
|
||||
```clojure
|
||||
;; bad
|
||||
(defn circle
|
||||
|
|
@ -245,24 +246,37 @@ Joshua Comeau.
|
|||
[rn/view {:style {:padding-horizontal 20}}]
|
||||
```
|
||||
|
||||
### Don't prepend booleans with is-
|
||||
### Use a question mark to convey the value is a boolean
|
||||
|
||||
It is a common practice in JavaScript and other languages to prepend boolean variable names with `is-*`.
|
||||
In ClojureScript it is common practice to suffix boolean variable names with a `?`.
|
||||
There is no need for both of these and so it is preferable to stick with the latter.
|
||||
The Clojure Style Guide suggests using a question mark only in [predicate
|
||||
functions](https://guide.clojure.style/#naming-predicates), but nothing is
|
||||
mentioned about other symbols and keywords. We prefer to extend the convention
|
||||
to all boolean references.
|
||||
|
||||
```clojure
|
||||
;; bad
|
||||
(let [is-open? true] ...)
|
||||
(def flag-is-enabled false)
|
||||
|
||||
;; good
|
||||
(let [open? true] ...)
|
||||
(def flag-enabled? false)
|
||||
```
|
||||
|
||||
And for keywords too:
|
||||
|
||||
```clojure
|
||||
;; bad
|
||||
[some-component {:logged-in true}]
|
||||
|
||||
;; good
|
||||
[some-component {:logged-in? true}]
|
||||
```
|
||||
|
||||
### Styles def vs defn
|
||||
|
||||
Always use `def` over `defn`, unless the style relies on dynamic values, such as
|
||||
deref'ed atoms.
|
||||
Always use `def` over `defn` if there are no dynamic values. This helps cut the
|
||||
cost of function calls.
|
||||
|
||||
```clojure
|
||||
;; bad
|
||||
|
|
@ -286,15 +300,18 @@ deref'ed atoms.
|
|||
|
||||
### Custom Colors
|
||||
|
||||
The Status designs have a lot of customization of user and group colors with components and pages. For consistency it is best to use `customization-color` as the prop key on pages and components. This will help easily identify what pages and components in the application are using customized colors.
|
||||
The Status designs have a lot of customization of user and group colors. For
|
||||
consistency it is best to use `customization-color` as the prop key on pages and
|
||||
components. This will help easily identify what pages and components in the
|
||||
application are using customized colors.
|
||||
|
||||
```clojure
|
||||
;; bad
|
||||
(defn community-card [{keys [custom-color]}]
|
||||
(defn community-card [{keys [custom-color]}]
|
||||
...)
|
||||
|
||||
;; good
|
||||
(defn community-card [{keys [customization-color]}]
|
||||
(defn community-card [{keys [customization-color]}]
|
||||
...)
|
||||
```
|
||||
|
||||
|
|
@ -518,9 +535,9 @@ the `quo2/` directory.
|
|||
|
||||
;; good
|
||||
(ns my-namespace
|
||||
(:require [quo2.core :as quo2]))
|
||||
(:require [quo2.core :as quo]))
|
||||
|
||||
(quo2/icon :i/verified)
|
||||
(quo/icon :i/verified)
|
||||
|
||||
;; also good because both namespaces are inside quo2/
|
||||
(ns quo2.components.tabs.account-selector
|
||||
|
|
@ -587,8 +604,7 @@ indices.
|
|||
|
||||
### Icons
|
||||
|
||||
Use the appropriate keyword qualification/namespace and don't directly require
|
||||
namespaces inside `quo2/components/`.
|
||||
Use the appropriate keyword qualification/namespace.
|
||||
|
||||
```clojure
|
||||
;; bad
|
||||
|
|
|
|||
|
|
@ -1,6 +1,6 @@
|
|||
#!/usr/bin/env sh
|
||||
|
||||
INVALID_CHANGES=$(grep -E -r '(re-frame/dispatch|rf/dispatch|re-frame/subscribe|rf/subscribe|rf/sub|<sub|>evt|status-im\.|status-im2\.)' './src/quo2')
|
||||
INVALID_CHANGES=$(grep -E -r '(re-frame/dispatch|rf/dispatch|re-frame/subscribe|rf/subscribe|rf/sub|<sub|>evt|status-im\.|status-im2\.)' --include '*.cljs' --include '*.clj' './src/quo2')
|
||||
|
||||
if test -n "$INVALID_CHANGES"; then
|
||||
echo "WARNING: re-frame, status-im are not allowed in quo2 components"
|
||||
|
|
|
|||
|
|
@ -0,0 +1,186 @@
|
|||
# Quo Library
|
||||
|
||||
Quo is the name of our mobile *component library* that implements the [Status
|
||||
Design System for Mobile](https://www.figma.com/file/WQZcp6S0EnzxdTL4taoKDv/Design-System-for-Mobile).
|
||||
|
||||
The overarching goals of having this component library are:
|
||||
|
||||
- Achieve the highest possible fidelity between code and the design system in
|
||||
Figma.
|
||||
- Decouple components from ever-changing business requirements.
|
||||
|
||||
> [!NOTE]
|
||||
> This document captures our current practices and guidelines for implementing
|
||||
> the design system. For guidelines that apply across the entire project, take
|
||||
> a look at [new-guidelines](/doc/new-guidelines.md).
|
||||
|
||||
## Directory structure and file names
|
||||
|
||||
We follow one basic rule: mirror Figma *pages* and their *component names* in
|
||||
the directory structure.
|
||||
|
||||
For example, in the screenshot below we see the Figma page is `Banners`
|
||||
and the component name is `Banner`.
|
||||
|
||||
<img src="/doc/images/quo-component.png" width="600" />
|
||||
|
||||
Therefore, the structure should look like:
|
||||
|
||||
```
|
||||
quo2/
|
||||
└── components/
|
||||
└── banners/
|
||||
└── banner/
|
||||
├── component_spec.cljs
|
||||
├── style.cljs
|
||||
└── view.cljs
|
||||
```
|
||||
|
||||
Files `view.cljs`, `style.cljs`, and `component_spec.cljs` should always have
|
||||
the same name, regardless of component.
|
||||
|
||||
## Component API
|
||||
|
||||
Adhere to the **same component properties and values** used in a Figma component
|
||||
when translating it to Clojure. This means using the same names for props and
|
||||
the same values. If the Figma property is a boolean, use a question mark suffix
|
||||
to make the name more idiomatic in Clojure.
|
||||
|
||||
We have found over time that the less we drift from the design system the
|
||||
better. Some key benefits:
|
||||
|
||||
- It helps developers quickly check for issues when comparing the code with the
|
||||
source of truth in Figma.
|
||||
- It is easier for pull-request reviewers to double-check components for
|
||||
correctness.
|
||||
- It helps developers create preview screens that are identical or very similar
|
||||
to Figma, which aids in spotting bugs more easily.
|
||||
- It helps designers review all component variations in preview screens.
|
||||
|
||||
<img src="/doc/images/figma-properties.png" width="600" />
|
||||
|
||||
In the image above we can see the properties are `Type`, `State`, `Size`,
|
||||
`Icon`, `Theme`, and `Background`. Translated to Clojure:
|
||||
|
||||
```clojure
|
||||
;; ns quo2.components.buttons.button.view
|
||||
(def view
|
||||
[{:keys [type state size icon theme background]}]
|
||||
...)
|
||||
```
|
||||
|
||||
## Clojure var conventions
|
||||
|
||||
- Due to the fact that every `view` namespace should export only one component
|
||||
and to avoid the redundancy of `[some-component/some-component ...]`, name the
|
||||
public var `view` as well.
|
||||
- Try to make all other vars private because they should almost never be used
|
||||
directly.
|
||||
|
||||
## Component tests
|
||||
|
||||
We don't attempt to write component tests verifying how components look on the
|
||||
screen. Instead, we have found a middle ground, where the focus is on verifying
|
||||
if events are triggered as intended and that all component variations are
|
||||
rendered. We use [React Native Testing Library](https://callstack.github.io/react-native-testing-library/).
|
||||
|
||||
There are dozens of examples in the repository, so use them as a reference. A
|
||||
good and complete example is [quo2.components.avatars.user-avatar.component-spec](/src/quo2/components/avatars/user_avatar/component_spec.cljs)
|
||||
|
||||
## Do not couple the library with re-frame
|
||||
|
||||
Don't use re-frame inside this library (e.g. dispatch & subscribe). If a
|
||||
component needs to be stateful, the state should be local to its rendering
|
||||
lifecycle (using `reagent.core/atom`). Additionally, if the component requires
|
||||
any other data, it should be passed as arguments.
|
||||
|
||||
```clojure
|
||||
;; bad
|
||||
(defn view []
|
||||
(let [window-width (rf/sub [:dimensions/window-width])]
|
||||
[rn/pressable {:on-press #(rf/dispatch [:do-xyz])}
|
||||
(do-something window-width)]))
|
||||
|
||||
;; good
|
||||
(defn view [{:keys [window-width on-press]}]
|
||||
[rn/pressable {:on-press on-press}
|
||||
(do-something window-width)])
|
||||
```
|
||||
|
||||
## Themes
|
||||
|
||||
Our goal is to make all design system components *themeable*, which means they
|
||||
should not use, nor fallback to the OS theme, because themes are *contextual*
|
||||
and can be overridden in specific parts of the app.
|
||||
|
||||
To achieve this, use the higher-order function `quo2.theme/with-theme` to
|
||||
automatically inject the current theme context (based on the [React Context
|
||||
API](https://react.dev/learn/passing-data-deeply-with-context)).
|
||||
|
||||
Use the following pattern:
|
||||
|
||||
```clojure
|
||||
(ns quo2.components.<figma page>.<component name>.view
|
||||
(:require [quo2.theme :as quo.theme]))
|
||||
|
||||
(defn- view-internal [{:keys [theme]}]
|
||||
...)
|
||||
|
||||
(def view (quo.theme/with-theme view-internal))
|
||||
```
|
||||
|
||||
Then pass the `theme` value down to all functions that may rely on the OS theme,
|
||||
like `quo2.foundations.colors/theme-colors` or `quo2.foundations.shadows/get`.
|
||||
|
||||
## Avoid using quo's version number in namespace aliases
|
||||
|
||||
When requiring quo2 namespaces, don't use the version number in the
|
||||
[alias](https://clojure.org/guides/learn/namespaces#_require), unless for a
|
||||
special reason you need to require both the old and new namespaces in the same
|
||||
file.
|
||||
|
||||
> [!NOTE]
|
||||
> Keep in mind that, at the moment, we need to keep both `src/quo/` and
|
||||
> `src/quo2/` directories in the repository, but eventually the old one will go
|
||||
> away and the version number will lose its meaning.
|
||||
|
||||
```clojure
|
||||
;; bad
|
||||
(ns ...
|
||||
(require [quo2.theme :as quo2.theme]
|
||||
[quo2.core :as quo2]))
|
||||
|
||||
;; good
|
||||
(ns ...
|
||||
(require [quo2.theme :as quo.theme]
|
||||
[quo2.core :as quo]))
|
||||
```
|
||||
|
||||
## Preview screens
|
||||
|
||||
Every component should be accompanied by a preview screen in
|
||||
`src/status_im2/contexts/quo_preview/`. Ideally, **all possible variations in
|
||||
Figma should be achievable in the preview screen** by changing the input values
|
||||
without resorting to code changes. Designers will also use this capability to
|
||||
review components in PR builds.
|
||||
|
||||
## Allow outermost containers to have their styles overridden
|
||||
|
||||
If a component needs to be wrapped in a `rn/view` instance to force it to be
|
||||
styled differently, consider changing the component to accept a
|
||||
`container-style` argument. This will help reduce the number of nodes to be
|
||||
rendered.
|
||||
|
||||
```clojure
|
||||
;; bad
|
||||
[rn/view {:style {:margin-right 12}}
|
||||
[quo/button
|
||||
{:size 32}
|
||||
:i/info]]
|
||||
|
||||
;; good
|
||||
[quo/button
|
||||
{:size 32
|
||||
:container-style {:margin-right 12}}
|
||||
:i/info]
|
||||
```
|
||||
Loading…
Reference in New Issue