Conversation
📝 WalkthroughWalkthroughRemplacement et renommage du composant de badges de filtres : Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/ui/src/design-system/BadgeFilters/BadgeFilters.stories.tsx`:
- Around line 108-109: Met à jour les libellés de la story interactive qui
utilisent encore "FilterBadges" : remplace "FilterBadges" par "BadgeFilters"
dans les propriétés title et description (les chaînes actuelles sous
title="Interactive FilterBadges (Typed)" et description="...FilterBadges...") du
fichier BadgeFilters.stories.tsx afin d’être cohérent avec le renommage de la
composante.
In `@packages/ui/src/design-system/BadgeFilters/index.tsx`:
- Around line 85-86: The code calls selectedFilters.sort(...) which mutates the
props array; change this to sort a copy instead (e.g., use
[...selectedFilters].sort(...) or selectedFilters.toSorted(...)) where the
rendering expression uses selectedFilters so the parent prop is not mutated;
update the expression that currently references selectedFilters.sort to operate
on a non-mutating copy.
🧹 Nitpick comments (4)
packages/ui/src/storybook/story.wrapper.tsx (1)
12-12: Le paragraphe de description est rendu même quanddescriptionestundefined.Cela produit un élément
<p>vide dans le DOM avec ses styles associés. Envisagez un rendu conditionnel.♻️ Proposition
- <p className="text-sm text-grey-8 mb-0">{description}</p> + {description && <p className="text-sm text-grey-8 mb-0">{description}</p>}packages/ui/src/design-system/BadgeFilters/index.tsx (2)
158-169: La logique deshouldShowClearAllFilterssemble contradictoire.
hasMoreThanOneRemovableCategoryexigelength > 1ethasOnlyOneRemovableCategoryexigelength === 1. Ces deux conditions sont mutuellement exclusives, donc!hasOnlyOneRemovableCategoryest toujours vrai quandhasMoreThanOneRemovableCategoryest vrai. La vérification!hasOnlyOneRemovableCategoryest redondante et prête à confusion.De plus, la dernière condition (ligne 169)
filterCategories.filter(c => !c.readonly).length > 0est forcément satisfaite siremovableCategories.length > 1.Est-ce l'intention ? Peut-être que la condition souhaitée est plus simple :
const shouldShowClearAllFilters = !!onClearAllFilters && removableCategories.length > 1;
118-140: Propdisabledinutilisée dansClearAllFiltersButton.Le type inclut
disabled?: boolean(ligne 123) mais il n'est jamais utilisé dans le composant.packages/ui/src/design-system/BadgeFilters/BadgeFilters.stories.tsx (1)
60-62: Le type decategoryKeyne couvre pas le casTKey[].
handleDeleteFilterValueattendcategoryKey: TaskFilterKeys(simple string), maisFilterBadgesProps.onDeleteFilterValuefournitcategoryKey: TKey | TKey[]. La comparaisoncategory.key === categoryKey(ligne 67) échouera sikeyest un tableau. C'est un fichier de stories donc l'impact est limité, mais cela pourrait induire en erreur un développeur qui s'en inspirerait.
packages/ui/src/design-system/BadgeFilters/BadgeFilters.stories.tsx
Outdated
Show resolved
Hide resolved
| {selectedFilters | ||
| .sort((a, b) => a.localeCompare(b)) |
There was a problem hiding this comment.
.sort() mute le tableau selectedFilters reçu en props.
Array.prototype.sort() trie le tableau en place. Ici, selectedFilters provient des props du composant parent, ce qui entraîne une mutation inattendue des données du parent. Utilisez [...selectedFilters].sort(...) ou .toSorted(...).
🐛 Proposition de correction
- {selectedFilters
- .sort((a, b) => a.localeCompare(b))
+ {[...selectedFilters]
+ .sort((a, b) => a.localeCompare(b))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {selectedFilters | |
| .sort((a, b) => a.localeCompare(b)) | |
| {[...selectedFilters] | |
| .sort((a, b) => a.localeCompare(b)) |
🤖 Prompt for AI Agents
In `@packages/ui/src/design-system/BadgeFilters/index.tsx` around lines 85 - 86,
The code calls selectedFilters.sort(...) which mutates the props array; change
this to sort a copy instead (e.g., use [...selectedFilters].sort(...) or
selectedFilters.toSorted(...)) where the rendering expression uses
selectedFilters so the parent prop is not mutated; update the expression that
currently references selectedFilters.sort to operate on a non-mutating copy.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/ui/src/design-system/BadgeFilters/BadgeFilters.stories.tsx`:
- Line 12: The Storybook title string is inconsistent with the component name;
update the exported title from 'Design System/BadgesFilters' to 'Design
System/BadgeFilters' so it matches the BadgeFilters component and Storybook
navigation; locate the title export in BadgeFilters.stories.tsx and adjust the
string accordingly.
In `@packages/ui/src/design-system/BadgeFilters/index.tsx`:
- Around line 207-237: The Tooltip currently shows interactive content (the
FilterByCategory list) but is activated on hover with a 0ms closing delay, which
makes its buttons hard to click; update the Tooltip around Badge in BadgeFilters
(the Tooltip wrapping Badge that renders categoriesLeft and FilterByCategory) to
either set activatedBy="click" or increase the closingDelay to a reasonable
value so the FilterByCategory buttons can be interacted with reliably (keep the
rest of the structure — VisibleWhen, Badge, FilterByCategory, onDelete handlers
— unchanged).
In `@packages/ui/src/design-system/Tooltip/Tooltip.tsx`:
- Around line 176-177: The eslint-disable comment is targeting the wrong rule:
replace the incorrect rule name `react-hooks/use-memo` with
`react-hooks/exhaustive-deps` on the line that suppresses the hook dependency
warning for the useCallback invocation (the line ending with `}, refs);`) so the
linter will actually suppress the exhaustive-deps warning for the `useCallback`
using `refs`.
🧹 Nitpick comments (3)
packages/ui/src/design-system/Tooltip/Tooltip.tsx (1)
106-108:safePolygon()est passé même quandactivatedBy === 'click'.Quand
activatedByvaut'click', c'estuseClickqui est utilisé. L'optionhandleCloseest spécifique àuseHoveret sera ignorée paruseClick. Ce n'est pas un bug, mais cela masque l'intention. Envisagez de ne passerhandleCloseque dans le casuseHover.♻️ Proposition
const { getReferenceProps, getFloatingProps } = useInteractions([ - (activatedBy === 'click' ? useClick : useHover)(context, { - delay: { open: openingDelay, close: closingDelay }, - handleClose: safePolygon(), - }), + activatedBy === 'click' + ? useClick(context) + : useHover(context, { + delay: { open: openingDelay, close: closingDelay }, + handleClose: safePolygon(), + }),packages/ui/src/design-system/BadgeFilters/index.tsx (2)
150-156: Le nom du type des props n'a pas suivi le renommage du composant.Le composant a été renommé de
FilterBadgesàBadgeFilters, mais le type des props est restéFilterBadgesProps(ligne 26). Pour la cohérence, renommez-le enBadgeFiltersProps.♻️ Proposition
-export type FilterBadgesProps<TKey extends string = string> = { +export type BadgeFiltersProps<TKey extends string = string> = {Et à la ligne 156 :
-}: FilterBadgesProps<TKey>) => { +}: BadgeFiltersProps<TKey>) => {
42-59: Le composantFilterpassechildrencomme valeur de la proptitle.
Badgeattendtitle?: React.ReactNode, etFilterpassechildrendanstitle. Cela fonctionne, mais le nommagechildrenpour un contenu qui est en réalité letitledu badge est trompeur. Considérez renommer entitleoulabelpour plus de clarté.
|
|
||
| const meta: Meta<typeof BadgeFilters> = { | ||
| component: BadgeFilters, | ||
| title: 'Design System/BadgesFilters', |
There was a problem hiding this comment.
Incohérence dans le titre Storybook : BadgesFilters vs BadgeFilters.
Le titre de la story est 'Design System/BadgesFilters' (avec un "s" supplémentaire), alors que le composant s'appelle BadgeFilters. Cela crée une incohérence dans la navigation Storybook.
✏️ Proposition
- title: 'Design System/BadgesFilters',
+ title: 'Design System/BadgeFilters',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| title: 'Design System/BadgesFilters', | |
| title: 'Design System/BadgeFilters', |
🤖 Prompt for AI Agents
In `@packages/ui/src/design-system/BadgeFilters/BadgeFilters.stories.tsx` at line
12, The Storybook title string is inconsistent with the component name; update
the exported title from 'Design System/BadgesFilters' to 'Design
System/BadgeFilters' so it matches the BadgeFilters component and Storybook
navigation; locate the title export in BadgeFilters.stories.tsx and adjust the
string accordingly.
| <VisibleWhen condition={hasMoreCategoriesToDisplay}> | ||
| <Tooltip | ||
| label={ | ||
| <div className="flex flex-col gap-2"> | ||
| {categoriesLeft.map(({ key, ...category }) => ( | ||
| <Fragment key={category.title}> | ||
| <FilterByCategory | ||
| title={category.title} | ||
| selectedFilters={category.selectedFilters} | ||
| readonly={category.readonly} | ||
| onlyShowCategory={category.onlyShowCategory} | ||
| onDeleteFilter={(valueToDelete) => { | ||
| onDeleteFilterValue({ categoryKey: key, valueToDelete }); | ||
| }} | ||
| onDeleteCategory={ | ||
| onDeleteFilterCategory | ||
| ? () => onDeleteFilterCategory(key) | ||
| : null | ||
| } | ||
| /> | ||
| </Fragment> | ||
| ))} | ||
| </div> | ||
| } | ||
| > | ||
| <Badge | ||
| title={`+${categoriesLeft.length}`} | ||
| state="standard" | ||
| className="px-2 py-1.5" | ||
| /> | ||
| </Tooltip> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the Tooltip component
fd -t f "Tooltip" --path "*/design-system/*" | grep -E "\.(tsx?|jsx?)$" | head -20Repository: incubateur-ademe/territoires-en-transitions
Length of output: 320
🏁 Script executed:
# Find the Badge component
fd -t f "Badge" --path "*/design-system/*" | grep -E "\.(tsx?|jsx?)$" | head -20Repository: incubateur-ademe/territoires-en-transitions
Length of output: 320
🏁 Script executed:
# Get the BadgeFilters file to verify the code state
cat -n packages/ui/src/design-system/BadgeFilters/index.tsx | sed -n '200,250p'Repository: incubateur-ademe/territoires-en-transitions
Length of output: 2000
🏁 Script executed:
# Find Tooltip component - search in design-system directory
find packages/ui/src/design-system -type f \( -name "*Tooltip*" -o -name "*tooltip*" \) | grep -E "\.(tsx?|jsx?)$"Repository: incubateur-ademe/territoires-en-transitions
Length of output: 310
🏁 Script executed:
# Find Badge component - search in design-system directory
find packages/ui/src/design-system -type f \( -name "*Badge*" -o -name "*badge*" \) | grep -E "\.(tsx?|jsx?)$"Repository: incubateur-ademe/territoires-en-transitions
Length of output: 449
🏁 Script executed:
# Also check directory structure
ls -la packages/ui/src/design-system/ | head -30Repository: incubateur-ademe/territoires-en-transitions
Length of output: 1774
🏁 Script executed:
# Read Tooltip component implementation
cat -n packages/ui/src/design-system/Tooltip/Tooltip.tsxRepository: incubateur-ademe/territoires-en-transitions
Length of output: 6153
🏁 Script executed:
# Read Badge component implementation
cat -n packages/ui/src/design-system/Badge/Badge.tsxRepository: incubateur-ademe/territoires-en-transitions
Length of output: 4548
Le contenu interactif dans le Tooltip nécessite activatedBy="click" ou un délai de fermeture augmenté.
Par défaut, le Tooltip s'active au hover avec un délai de fermeture de 0ms. Même avec safePolygon(), l'utilisateur doit naviguer précisément vers le tooltip pour interagir avec les boutons de suppression. Avec un contenu interactif (FilterByCategory avec boutons de suppression), utilisez activatedBy="click" ou augmentez le closingDelay pour que les filtres soient réellement cliquables.
De plus, le composant Badge utilise correctement forwardRef pour que Tooltip positionne correctement le floating element — bon alignement.
🤖 Prompt for AI Agents
In `@packages/ui/src/design-system/BadgeFilters/index.tsx` around lines 207 - 237,
The Tooltip currently shows interactive content (the FilterByCategory list) but
is activated on hover with a 0ms closing delay, which makes its buttons hard to
click; update the Tooltip around Badge in BadgeFilters (the Tooltip wrapping
Badge that renders categoriesLeft and FilterByCategory) to either set
activatedBy="click" or increase the closingDelay to a reasonable value so the
FilterByCategory buttons can be interacted with reliably (keep the rest of the
structure — VisibleWhen, Badge, FilterByCategory, onDelete handlers —
unchanged).
cparthur
force-pushed
the
TET-6714/DS-BadgeFilters
branch
from
dee8927 to
24437bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/app/src/ui/lists/DEPRECATED_filter-badges/use-filters-to-badges.ts (1)
20-21:⚠️ Potential issue | 🟡 MinorConflit de nommage confirmé entre le type et le composant
BadgeFilters.Le type
BadgeFiltersexporté depuisapps/app/src/ui/lists/DEPRECATED_filter-badges/use-filters-to-badges.ts(ligne 20) porte le même nom que le composantBadgeFiltersexporté depuispackages/ui/src/design-system/BadgeFilters/index.tsxet réexporté via@tet/ui. Un développeur tentant d'importer les deux dans le même fichier verrait la deuxième importation écraser la première, causant des erreurs imprévues.Envisagez de renommer ce type (par ex.
DEPRECATED_BadgeFiltersouBadgeFiltersType) pour éviter les collisions d'import.
🤖 Fix all issues with AI agents
In `@packages/ui/src/storybook/story.wrapper.tsx`:
- Line 12: Le <p className="text-sm text-grey-8 mb-0">{description}</p> est
rendu même si description est undefined; changez le rendu pour ne créer le <p>
que quand description est truthy (par ex. utiliser un rendu conditionnel autour
de description) afin d'éviter un <p> vide et l'espacement indésirable dans
story.wrapper.tsx.
🧹 Nitpick comments (5)
packages/ui/src/design-system/Badge/Badge.tsx (2)
1-1: Utilisation mixte declassNamesetcndans le même composant.
classNames(ligne 1) n'est utilisé qu'à la ligne 92, alors quecnest utilisé partout ailleurs. Sicnencapsuletailwind-merge, les classes Tailwind passées viaclassNamesà la ligne 92 ne bénéficieront pas de la résolution de conflits. Pour la cohérence et éviter des bugs subtils de styles, il serait préférable d'utilisercnpartout et de retirer l'importclassNames.♻️ Remplacement suggéré
-import classNames from 'classnames'; - -import { Icon, IconValue } from '../Icon'; +import { Icon, IconValue } from '../Icon';<span - className={classNames( + className={cn( styles.text, 'font-bold leading-4 text-left', {Also applies to: 92-101
107-121: Le bouton de fermeture (<div>) manque d'accessibilité clavier.Le
<div>aveconClick(ligne 108-113) n'est pas accessible au clavier : pas derole="button", pas detabIndex, pas de gestiononKeyDown. Même si c'est probablement un comportement pré-existant, c'est l'occasion de l'améliorer puisque le composant est retravaillé.♿ Suggestion d'amélioration
- <div - className="flex rounded-full cursor-pointer" - onClick={(evt) => { + <button + type="button" + className="flex rounded-full cursor-pointer border-none bg-transparent p-0" + onClick={(evt) => { evt.stopPropagation(); onClose?.(); }} > <Icon icon="close-circle-line" size={size === 'sm' ? 'xs' : 'sm'} className={cn(styles.icon)} /> - </div> + </button>packages/ui/src/design-system/Tooltip/Tooltip.tsx (1)
105-113:safePolygon()n'est pertinent que pouruseHover, pas pouruseClick.Quand
activatedBy === 'click',useClickest utilisé et l'optionhandleCloseest ignorée. Ce n'est pas un bug (c'est inoffensif), mais pour la clarté, il serait préférable de ne passerhandleCloseque dans le cas hover.♻️ Proposition
const { getReferenceProps, getFloatingProps } = useInteractions([ - (activatedBy === 'click' ? useClick : useHover)(context, { - delay: { open: openingDelay, close: closingDelay }, - handleClose: safePolygon(), - }), + (activatedBy === 'click' ? useClick : useHover)(context, { + delay: { open: openingDelay, close: closingDelay }, + ...(activatedBy === 'hover' && { handleClose: safePolygon() }), + }),apps/app/src/app/pages/CollectivitesEngagees/header/collectivites-header.tsx (1)
8-8: Migration vers le nouveauBadgeFiltersà planifier.L'import pointe désormais vers le chemin déprécié
DEPRECATED_filter-badges. C'est cohérent avec l'approche transitoire de cette PR, mais il serait bon de prévoir la migration deDeleteFiltersButtonvers le nouveau composantBadgeFilters(ou un équivalent) dans un prochain ticket pour éviter l'accumulation de dette technique.apps/app/src/tableaux-de-bord/modules/module/module.tsx (1)
4-7: Ambiguïté potentielle de nommage avec le composantBadgeFiltersde@tet/uilors de la future migration.Le type
BadgeFiltersimporté depuis le module déprécié (ligne 5) porte le même nom que le composant génériqueBadgeFiltersexporté par@tet/ui. Bien qu'aucun conflit n'existe actuellement dans ce fichier, renommer ce type (par exemple enModuleFilterConfig,ModuleFilters, ouDeprecatedBadgeFilters) faciliterait l'adoption du nouveau composant lors de la migration de ce module vers les patterns de filtrage du design system.
| <div className="flex flex-col gap-4"> | ||
| <div className="flex flex-col gap-1"> | ||
| <h3 className="text-lg font-semibold">{title}</h3> | ||
| <p className="text-sm text-grey-8 mb-0">{description}</p> |
There was a problem hiding this comment.
Le paragraphe de description est rendu même quand description est undefined.
Cela produit un <p> vide dans le DOM avec du style appliqué, ce qui peut ajouter un espacement non souhaité.
🐛 Proposition
- <p className="text-sm text-grey-8 mb-0">{description}</p>
+ {description && <p className="text-sm text-grey-8 mb-0">{description}</p>}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <p className="text-sm text-grey-8 mb-0">{description}</p> | |
| {description && <p className="text-sm text-grey-8 mb-0">{description}</p>} |
🤖 Prompt for AI Agents
In `@packages/ui/src/storybook/story.wrapper.tsx` at line 12, Le <p
className="text-sm text-grey-8 mb-0">{description}</p> est rendu même si
description est undefined; changez le rendu pour ne créer le <p> que quand
description est truthy (par ex. utiliser un rendu conditionnel autour de
description) afin d'éviter un <p> vide et l'espacement indésirable dans
story.wrapper.tsx.
1 participant
J'ai voulu remplacer et supprimer
BadgesContainerqui est défini dans le dossierBadgedu design system.Malheureusement, tous les composants utilisant
BadgesContainerdépendent du hookuseFiltersToBadgesqui ne renvoie pas les données au format attendu parBadgeFilters.J'ai donc passé
BadgesContaineret les composants associés enDEPRECATEDuseFiltersToBadgesagrège tous les filtres possible de l'app et non les filtres de chaque entité (mesures, actions, indicateurs ...) en renvoyant directement un string alors queBadgeFiltersattend des catégories avec deskeysprécisent et des méthodes pour modifier ces catégories.Il faut aller voir
plan-filters.context.tsxoufiche-action-filters-context.tsxpour se rendre compte de ce qu'il y a à faire.Personnellement je trouve ça plutôt compliqué et il y a pas mal de travail fait en double car pour des entités différentes, on peut retrouver les mêmes filtres (personne pilotes, dates, service pilote, ...).
Je pense qu'il faudrait trouver un juste milieu mais clairement hors scope de cette PR.
Summary by CodeRabbit
Nouvelles fonctionnalités
Refactor
Style / UX