Page groupe d’indicateurs sous forme de liste #3531
Conversation
| } | ||
| return a.identifiant.localeCompare(b.identifiant); | ||
| }); | ||
| const SousIndicateurs = ({ enfantsIds }: Props) => { |
There was a problem hiding this comment.
Super d'avoir enlevé de la logique de fetch du composant 👌
| /> | ||
| </button> | ||
| {displayExportButton && ( | ||
| <ExportButton |
There was a problem hiding this comment.
Idem, cool d'avoir sorti la logique de l'export dans un component séparé
|
|
||
| type Props = { | ||
| pageName: ExportIndicateursPageName; // tracking | ||
| pageName?: ExportIndicateursPageName; // tracking |
There was a problem hiding this comment.
Pas une remarque mais une question de curiosité : pourquoi avoir mis pageName en optionnel ?
There was a problem hiding this comment.
pageName est utilisé pour tracker l'export via le bouton présent dans la liste de badges, mais je n'utilise pas ce bouton donc ce n'était pas très pertinent de conserver cette prop en obligatoire. Dans mon cas, la pageName pour le tracking est définie dans le composant <IndicateurToolbar/> qui contient le bouton d'export.
There was a problem hiding this comment.
Pas une question directement liée à ce commit, mais plus généralement, pourquoi est-ce que les noms des fichiers du dossier indicateurs-list sont en kebab-case, et non en camelCase ? J'imagine que c'est une convention de nommage de l'équipe, si c'est le cas peux-tu me dire la règle, s'il te plaît ?
There was a problem hiding this comment.
Je ne saurais pas te dire, pour moi il n'y a pas encore eu de consensus sur les conventions de nommage, c'est encore un sujet en cours
mariheck
force-pushed
the
TET-3930/page-groupe-indicateurs
branch
2 times, most recently
from
fd8652d to
185fe4a
Compare
mariheck
force-pushed
the
TET-3930/page-groupe-indicateurs
branch
from
e1fe2a3 to
cb5bddd
Compare
Ticket https://www.notion.so/accelerateur-transition-ecologique-ademe/Cr-er-page-groupe-d-indicateurs-sous-forme-de-liste-cas-des-groupements-d-indicateurs-1376523d57d78076a244c974d6742b59?pvs=4