Commit b5d9ee4c authored by Camille Rabatel's avatar Camille Rabatel
Browse files

doc: Add new code review

parent af5df9fb
Code Review
===========
Certains points ont été repris de l'audit sur MLP.
De manière générale, peu de choses à dire sur TypeScript qui est très bien utilisé.
Quelques points de performances sur React devraient être traités pour améliorer celles-ci.
L'accessibilité souffre aussi de quelques oublis, ce pourrait être intéressant d'étudier plus en profondeur ce sujet.
## SVG
### Optimisation SVGOMG
Pour optimiser le poids des SVG, il est intéressant de les passer dans l'utilitaire en ligne [SVGOMG](https://jakearchibald.github.io/svgomg/).
Cet utilitaire nettoie ce qui n'est pas nécessaire, fusionne certains paths quand il peut, et permet d'obtenir un SVG plus propre.
* Exemple : pour `src/assets/icons/ico/switchGRDFMobile.svg`, 34% de réduction de poids.
### Optimisation Illustrator
Sur Illustrator, ou d'autres éditeurs SVG avancés, il est possible de fusionner les chemins SVG pour en obtenir un seul et unique dans le meilleur des cas.
Cela peut être intéressant pour réduire le nombre d'éléments du SVG, mais ne doit pas être utilisé systèmatiquement si le SVG doit avoir du style spécifique pour certains de ses éléments, comme une couleur différente.
## CSS
### Media Queries
Bon usage des media queries ;)
### Sélecteurs imbriqués
De manière générale, il faut éviter d'utiliser la notation SCSS permettant d'imbriquer des sélecteurs.
Par exemple dans `src/components/CommonKit/Modal/Modal.scss` :
```scss
.modal-overlay {
// ...
.modal-box {
// ...
.modal-close-button {
// ...
}
```
Car cela produit des sélecteurs finaux inutilement longs avec beaucoup de parties :
```css
.modal-overlay .modal-box .modal-close-button {
/* ... */
```
Or, plus un sélecteur a de parties, plus il sera long à être interprété et appliqué par le navigateur.
De plus, cela donne une priorité très élevée au sélecteur, et rend la surcharge compliquée, nécessitant soit le même nombre de classes dans cette exemple, soit un ID ou la notation `!important`.
La bonne pratique est de ne pas dépasser 3 sélecteurs, quelques soient leur priorité (class, id, élément, attribut, etc.).
Une manière assez simple et connue pour atteindre cet objectif est de respecter le plus possible la méthode [BEM](http://getbem.com/introduction/).
Cela permet de simplifier les sélecteurs, en ayant généralement qu'une seule classe, voire deux lorsqu'on a besoin de modificateurs, et parfois un sélecteur d'élément à la fin.
Dans l'exemple ci-dessus, on peut assez facilement appliquer le principe de BEM car les classes sont déjà bien définies et on un préfixe `modal` qui peut servir de nom de bloc. et on pourrait donc écrire directement dans le SCSS :
```scss
.modal-overlay {
// ...
}
.modal-box {
// ...
}
.modal-close-button {
// ...
}
```
## Javascript
### Formatage d'un nombre avec sa devise
Dans `src/utils/utils.ts`, une fonction `formatNumberValues` est définie permettant de formatant un nombre.
Elle utilise bien l'API Intl native `toLocaleString`, par contre le résultat de cette fonction est parfois concaténé au symbol "€" comme dans `src/components/ConsumptionVisualizer/DataloadConsumptionVisualizer.tsx` ligne 85.
Pourtant `toLocaleString` permet aussi de formater automatiquement un nombre selon une devise, en lui rajoutant les options suivantes :
```js
value.toLocaleString('fr-FR', { style: 'currency', currency: 'EUR' })
// → 1 234 567,89 €
```
Ppeut-être que cette possibilité était connue mais qu'elle ne correspondait pas au besoin du métier ?
A noter que dans les cas particuliers où l'on doit afficher par exemple `---- €`, on peut utiliser la méthode `formatToParts` d'une instance de `Intl.NumberFormat` pour afficher le symbole au bon endroit automatiquement :
```ts
const formatter = new Intl.NumberFormat('fr-FR', {
style: 'currency',
currency: 'EUR'
});
const parts10 = formatter.formatToParts(10)
parts10.join('')
// → 10,00 €
const partsFake = parts10.map(({type, value}) => {
switch (type) {
case 'currency': return value;
default : return '-';
}
})
partsFake.join('');
// → ----- €
```
### Conditions complexes
Dans le cas de conditions complexes au sein du JSX, il est intéressant de déporter ces conditions dans une fonction séparée, ou de la stocker dans une variable, permettant de mieux expliquer quel rôle elle remplie exactement.
Par exemple dans `src/components/ConsumptionVisualizer/ConsumptionVisualizer.tsx` :
```tsx
{dataload &&
dataload.valueDetail &&
((dataload.valueDetail[0] === -1 &&
!dateChartService.isDataToCome(dataload, fluidTypes[0])) ||
(dataload.valueDetail[1] === -1 &&
!dateChartService.isDataToCome(dataload, fluidTypes[1])) ||
(dataload.valueDetail[2] === -1 &&
!dateChartService.isDataToCome(dataload, fluidTypes[2]))) && (
<ErrorDataConsumptionVisualizer
date={date}
indexDisplayed={indexDisplayed}
setIndexDisplayed={setIndexDisplayed}
lastDateWithAllData={lastDateWithAllData}
setSelectedDate={setSelectedDate}
/>
)}
```
L'expression n'est pas évidente à lire, et il est difficile de comprendre à quel cas elle correspond, rendant la maintenance et la reprise plus difficile.
On peut la remplacer par une variable nommée définie avant le rendu par exemple.
## React
### Instanciation à chaque rendu
Dans plusieurs composants, un service est instancié et utilisé directement dans la fonction de rendu.
Cela peut poser des problèmes performances, car le service sera instancié à chaque rendu du composant, or un composant est re-rendu dès que lui ou un de ses parents reçoit un nouveau state/context/prop (sauf s'il est décoré par le HOC `React.memo`).
Par conséquent, un nouveau service est instancié à chaque rendu, comme ici dans `src/components/Charts/AxisBottom.tsx` ligne 20 (voir ligne 125 aussi) :
```tsx
function TextAxis(props: TextTypeProps) {
// ...
const dateChartService = new DateChartService()
const isSelectedDate = dateChartService.compareStepDate(
timeStep,
selectedDate,
dataload.date
)
// ...
```
Pour éviter cela, il y a plusieurs façons de faire.
1. Instancier le service en dehors du composant, pratique mais pas recommandé car l'instance est conservé en mémoire même quand le composant n'existe plus sur la page :
```tsx
const dateChartService = new DateChartService()
function TextAxis(props: TextTypeProps) {
// ...
const isSelectedDate = dateChartService.compareStepDate(
timeStep,
selectedDate,
dataload.date
)
```
2. Avec le hook `useMemo`, si on a besoin du service qu'une seule fois :
```tsx
const isSelectedDate = useMemo(() => {
const dateChartService = new DateChartService()
return dateChartService.compareStepDate(
timeStep,
selectedDate,
dataload.date
)
}, [timeStep, selectedDate, dataload.date])
```
3. Avec le hook `useRef`, si le service pourrait être utilisé plusieurs fois :
```tsx
const dateChartServiceRef = useRef()
if (!dateChartServiceRef.current) {
dateChartServiceRef.current = new DateChartService()
}
const isSelectedDate = dateChartServiceRef.current.compareStepDate(
timeStep,
selectedDate,
dataload.date
)
```
### Calculs complexes
Dans les composants graphiques, il serait intéressant de mémoiser les résultats des fonctions de calcul, comme les fonctions d'initialisation des axes dans `src/components/Charts/BarChart.tsx`.
En effet ils sont exécutés à chaque rendu cela doit certainement peser dans les performances de l'application.
Pour éviter cela, on peut intégrer toutes ces fonctions dans un `useMemo`, pour être certain que le calcul n'est pas effectué de nouveau tant que les props n'ont pas changé :
```tsx
const { xScale, yScale } = useMemo(() => {
const getContentWidth = () => {
// ...
}
const getContentHeight = () => {
// ...
}
const getMaxLoad = () => {
// ...
}
const xScale: ScaleBand<string> = // ...
const yScale: ScaleLinear<number, number> = // ...
return {
xScale,
yScale,
}
}, [ /* ne pas oublier les dépendances ici */ ])
```
Vu aussi dans `src/components/SingleFluid/SingleFluidIndicators.tsx` ligne 50.
### Composants complexes
Certains composant sont particulièrement longs et complexes, gérant beaucoup de states et beaucoup de rendu à la fois.
Une refactorisation assez simple consiste à séparer en plusieurs composants, ou hooks, le fonctionnel et l'interface, c'est ce qu'on appelle parfois le pattern Conteneur/Présentation.
De cette façon, le state et le rendu du fonctionnel se retrouvent séparés du state et du rendu de l'interface.
Les composants suivants ont l'air de pouvoir être séparés de cette façon :
* `src/components/ContentComponents/KonnectorViewer/KonnectorViewerCard.tsx`
* `src/components/Connection/ConnectionResult.tsx`
## Services
## Tests
### Constat global
Jest a été utilisé pour l'écriture des tests, et à première vue les services ont des tests unitaires très complets, de même qu'une bonne partie des composants via l'utilisation d'Enzyme 👍
* Bon usage des mocks.
* Bon usage de `mount` pour vérifier le rendu réel d'un composant.
* Bon usage de `shallow` pour vérifier le retour du rendu d'un composant.
### Mocks redondants
Il serait intéressant de mutualiser le mock de `cozy-ui/transpiled/react/I18n` comme il est très utilisé, via une fonction de préparation de Jest.
### Snapshots
Des snaphots ont été utilisés sur la plupart des composants, ce qui est très pratique pour être sûr qu'un rendu n'est pas modifié sans le vouloir, toutefois ça peut être assez embêtant lorsque le rendu est modifié intentionnellement, ne serait-ce que pour un fix, les tests étant alors automatiquement faux.
Une façon plus ciblée permettant d'éviter l'invalidation complète du test lors d'une petite modification est de faire comme ça a été fait à certains endroits, c'est-à-dire de vérifier que le rendu du composant comporte bien les éléments critiques pour qu'il remplisse son rôle, sans chercher à vérifier tous les éléments du composant.
## TypeScript
### Retour de fonctions
La plupart des fonctions ont bien un type de retour défini ;)
Au cas où, voici ce que j'avais écrit pour l'audit de MPS sur ce sujet :
TypeScript est assez intelligent pour déduire automatiquement le type de retour selon les différentes instructions `return` présentes dans la fonction.
C'est très pratique pendant le développement pour éviter de se poser trop de questions sur les types.
Toutefois, dès le début du développement ou lors d'une refacto, il est préférable de préciser le type de retour pour plusieurs raisons :
* Plus TypeScript a besoin de faire des déductions, plus la vérification des types lors du build sera longue,
* Idem pour la vérification des types via l'IDE.
* Si le type de retour est présent et que la fonction renvoie un mauvais type, l'erreur de typage sera visible dans le corps de la fonction directement et sera facilement identifiable.
* Si le type de retour est absent, l'erreur de typage sera propagée à tous les endroits où la fonction est utilisée, et il peut devenir difficile de comprendre la nature et l'origine de l'erreur.
## Accessibilité
Rappel de bonnes pratiques pour permettre aux utilisateurs de lecteurs d'écrans ou de la navigation au clavier d'utiliser tous les éléments de l'interface.
### Bouton
Lorsque qu'un `div` ou tout autre élément que `button` est utilisé comme un bouton, il convient de rajouter les attributs nécessaires pour le lecteur d'écran ainsi que de traiter l'activation au clavier par les touches Entrée et Espace, et permettre la tabulation, comme le fait un bouton natif.
Vu dans :
`src/components/ConsumptionVisualizer/ErrorDataConsumptionVisualizer.tsx`
Exemple utilisable :
https://www.w3.org/TR/2019/NOTE-wai-aria-practices-1.1-20190814/examples/button/button.html
### Modale
Concernant les modales, il est intéressant de rajouter des attributs permettant d'identifier la modale, et de faire le lien entre le bouton qui l'active et la modale elle-même, en plus de la décrire plus précisément.
Vu dans :
`src/components/CommonKit/Modal/Modal.tsx`
Exemple utilisable :
https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/dialog.html
### Accordéon
De la même façon pour les accordéons, l'activation d'un panneau doit être géré par un bouton natif idéalement, et le panneau actif devrait être identifié et relié au bouton.
Vu dans :
`src/components/Konnector/KonnectorViewerCard.tsx`
Exemple utilisable :
https://www.w3.org/TR/wai-aria-practices/examples/accordion/accordion.html
\ No newline at end of file
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment