codegouvfr / react-dsfr

🇫🇷 The French Government Design system React toolkit
https://react-dsfr.codegouv.studio
MIT License
419 stars 57 forks source link

Notice - Améliore la réutilisation #342

Closed martinratinaud closed 4 days ago

martinratinaud commented 6 days ago

Bonjour, pour les besoins de France Chaleur Urbaine, nous utilisons extensivement le DSFR react et proposons les choses suivantes pour améliorer la réusabilité du composant Notice

Merci

garronej commented 5 days ago

Bonjour @martinratinaud,

Merci pour tes suggestions.

Je n'ai pas écrit ce composant, donc je le découvre en parcourant cette issue. Bien que le typage des props soit difficile à lire (j'ai d'ailleurs fait un commit pour l'améliorer), je trouve que le design de l'API de ce composant est globalement satisfaisant.

Supprimer isClosable -> s'il y a un onClose, cela paraît suffisant pour afficher le bouton de fermeture

Il peut être pertinent de rendre la notice closable sans pour autant nécessiter un callback pour une action à exécuter lors de sa fermeture.

Supprimer title et le remplacer par children, permettant ainsi d'ajouter des liens ou d'utiliser une autre casse

Le type de title est ReactNode, donc il ne s'agit pas nécessairement d'un string. Effectivement, utiliser children à la place de title serait plus esthétique, mais cela reste une considération mineure.

Écrire :

<Notice title={<Link>MonLien</Link>} /> 

ou

<Notice>
    <Link>MonLien</Link>
</Notice>

revient au final au même, et je ne pense pas que cela justifie un breaking change.

Ajouter un onClick -> aujourd'hui, il est difficile d'ajouter un lien dans ce composant, bien que nous l'utilisions en haut de page pour présenter une nouvelle fonctionnalité

Un onClick serait souvent trop restrictif. On veut éviter d'inciter les développeurs à écrire quelque chose comme :

<Notice 
   onClick={() => {
     location.href = routes.profile({ tabId: "notification" }).link.href;
   }}  
   title="Vous avez une nouvelle notification"
/>

Cela rechargerait l'application en entier lors du clic, et l'utilisateur ne pourrait pas utiliser le clic droit, ouvrir dans un nouvel onglet, ou copier l'adresse du lien.

Nous préférons quelque chose comme :

<Notice
    title={(
        <a {...routes.profile({ tabId: "notification" }).link}>
            Vous avez une nouvelle notification
        </a>
    )}
/>

(Ici, j'utilise un exemple avec TypeRoute, mais cela dépend du système de routage en place.)

Transmettre l'événement de clic au onClose

Ok, je l'ajoute, mais je suis un peu hésitant. L'API JS de DSFR (vanilla) ne permet pas de fermer impérativement ce composant, donc, actuellement, la fermeture se fait nécessairement via un événement de clic.

Cependant, il serait envisageable que la fermeture puisse aussi se faire via dsfr(element).notice.conceal(). Dans ce cas, il n'y aurait pas d'événement de clic à transmettre au callback. Je modifierai l'API si celle de DSFR évolue.

Garder en tête que vous êtes tout a fait libre de copier coller le code du composant dans votre codebase et de le modifier pour qu'il s'adapte a vos besoin spéficique.

Il suffit juste de remplacer les import relatifs avec des imports absolut:

-import { fr } from "./fr";
+import { fr } from "@codegoufr/react-dsfr/fr";
-import { cx } from "./tools/cx";
+import { cx } from "@codegouvfr/react-dsfr/tools/cx";
...
martinratinaud commented 4 days ago

Ok, c'est noté, merci pour ces réponses super détaillées