Ajout du fil d'ariane aux composants design system #3247
Conversation
| @@ -11,7 +11,7 @@ const buttonSizeToIconSize: Record<ButtonSize, IconSize> = { | |||
|
|
|||
| type BreadcrumbsProps = { | |||
| /** Liste des boutons à afficher dans le fil d'ariane */ | |||
| buttons: { | |||
| buttons?: { | |||
There was a problem hiding this comment.
Pourquoi avoir rendu boutons optionnels ?
S'il n'y a pas d'élément, autant de ne pas afficher le composant
| @@ -37,13 +45,15 @@ export const Breadcrumbs = ({buttons, size, onClick}: BreadcrumbsProps) => { | |||
| className="flex items-center shrink-0 gap-x-1" | |||
| > | |||
| <Button | |||
There was a problem hiding this comment.
Pourquoi ne pas avoir garder l'ancienne structure link.path ? <Link /> : <div/> ?
Ca permet de ne pas avoir à styliser le Button pour fake un aspect disabled ou de disabled un bouton tout court.
D'ailleurs ça ne devrait pas être des Button mais des Link, car un bouton avec href, cela créé un <a> qui lorsque l'user click dessus, recharge toute la page au lieu de se servir du router de l'app. J'ai noté ça sur les compos avec des boutons et href de setup dessus, faut absolument qu'on trouve une solution.
Sur le nommage je mettrais links ou items pour etre plus générique à la place de buttons .
There was a problem hiding this comment.
Je ne peux pas utiliser le composant <Link/>, car selon le projet il sera importé soit de react-router-dom, soit de next/link. D'ailleurs la balise <Link/> crée aussi une balise <a/>. Et je ne pense pas qu'il faille s'appuyer sur react-router-dom dans les composants DS alors qu'on prépare la migration vers next.
Pour les boutons avec href on pourrait peut être régler le problème avec router.push ou history.push depuis le onClick en attendant de pouvoir l'intégrer directement au composant après la migration de l'app (je n'étais pas encore tombée sur ce problème).
Sinon j'ai utilisé le composant Button partout pour réutiliser ce qui est déjà fait en terme de fonctionnalité avec href et onClick, mais aussi pour utiliser les mises en formes déjà faites (padding, taille de texte et espacements en fonction de la taille de bouton choisie). Si j'utilise une <div/> je dois refaire toute la mise en forme, c'est plus simple de mettre un bouton en disabled.
Ok pour le nommage, je remplace par items du coup.
There was a problem hiding this comment.
Je sais c'est relou pour les balises <Link /> :/
Oui je pense que c'est une bonne option le onClick avec le fonction de framework donnée par l'app qui l'utilise.
Tu fais un test avec ce compo?
okok pour la <div /> 👍
There was a problem hiding this comment.
J'ai fait la mise à jour, ça a l'air de bien marcher 🙂
| buttons: { | ||
| label: string; | ||
| href?: string; | ||
| onClick?: () => void; |
There was a problem hiding this comment.
Il me semble qu'un balise <a/> n'a pas de props onClick et je ne vois pas pourquoi l'on voudrait afficher un bouton à la place d'un lien dans un fil d'ariane.
À moins que ce soit parce qu'il existait un compo FilArianeButtons, mais il est utilisé dans un dossier old alors je ne voudrais pas que l'on développe des choses pour être retro-compatibles avec du code que l'on va supprimer.
À la limite tu peux garder les vieux fil d'ariane dans le dossier charts/old pour que l'autre continue de marcher.
There was a problem hiding this comment.
Le composant <BarChartCardWithSubrows/> ne devrait pas être dans charts/old selon moi. Même si on revoit la carte pour se caler aux autres graphiques, on aura besoin du fil d'ariane pour naviguer entre les différents axes et actualiser les données du graphes. Et pour le moment ce graphique est utilisé à deux endroits de l'app sans remplacement prévu à moyen terme à ma connaissance
Pour le onClick, je l'ai mis au niveau de chaque item / button pour des utilisations comme dans mon commentaire au dessus, mais aussi au niveau du composant avec renvoi de l'index sélectionné pour des utilisations comme dans le graphe où un href ne répond pas au besoin.
There was a problem hiding this comment.
À mon sens il doit être dans old car il utilise l'ancien composant ChartCard.
Okok pour les onClick 👍
There was a problem hiding this comment.
Ça marche, mais dans ce cas là il faut garder en tête que ce n'est pas un composant amené à disparaitre, mais à refactoriser...
| buttons: { | ||
| label: string; | ||
| href?: string; | ||
| onClick?: () => void; |
There was a problem hiding this comment.
Il me semble qu'un balise <a/> n'a pas de props onClick et je ne vois pas pourquoi l'on voudrait afficher un bouton à la place d'un lien dans un fil d'ariane.
À moins que ce soit parce qu'il existait un compo FilArianeButtons, mais il est utilisé dans un dossier old alors je ne voudrais pas que l'on développe des choses pour être retro-compatibles avec du code que l'on va supprimer.
À la limite tu peux garder les vieux fil d'ariane dans le dossier charts/old pour que l'autre continue de marcher.
| onClick?: () => void; | ||
| }[]; | ||
| /** Click détecté sur un des boutons, avec envoi de son index */ | ||
| onClick?: (index: number) => void; |
There was a problem hiding this comment.
Je ne comprends pas trop pourquoi mettre un onClick à ce niveau là.
Je vois que tu l'utilises dans une story, si c'est juste pour ce cas d'usage alors il ne faut pas polluer les composants du DS avec des propriétés de test
There was a problem hiding this comment.
Ce n'est pas une propriété de test, il est utilisé de cette manière dans le composant <BarChartCardWithSubrows/>, et permet de connaitre l'item sélectionné depuis la composant parent dans le cas où un href ne répond pas au besoin.
No description provided.