fix(components): make banner visible again when isClosable or isColla…

[lwih]

…psible change (rapportnav usecase)

Description

Côté RapportNav, on peut passer d'un type de banniere à l'autre selon certains cas. Mais la transition était mal supportée comme la video l'indique.

Screen.Recording.2024-04-24.at.12.51.53.mov

Preview URL

https://637e01cf5934a2ae881ccc9d-sxfnedzwrg.chromatic.com/

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.39%. Comparing base (76d3057) to head (8b32035).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1155   +/-   ##
=======================================
  Coverage   57.39%   57.39%           
=======================================
  Files          55       55           
  Lines         805      805           
  Branches      274      274           
=======================================
  Hits          462      462           
  Misses        327      327           
  Partials       16       16           

Flag	Coverage Δ
unit	57.39% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[louptheron]

On ne re-init pas avec !!isHiddenByDefault ?

[lwih]

Tu peux faire ça aussi mais mon usecase est différent :
côté rapportnav, quand le rapport est incomplet, on est sur une Banner qui se rétracte mais quand le rapport devient complet, elle devient fermable. Et dans ce cas là, si elle avait été rétractée, alors elle restait rétractée alors qu'on la veut full-height fermable.

[ivangabriele]

Alors je challenge l'existence même de ce useEffect : pour moi une Banner n'est pas sensée être un composant qu'on garde dans le DOM en faisant évoluer ses props, c'est un composant temporaire qui peut avoir plusieurs cas d'utilisation et qui devrait être détruit et recréé par l'app qui l'utilise à chaque fois qu'on couvre un autre "cas". Soit via une key soit via un {myState1 && <Banner ...>} {myState2 && <Banner ...>}.

Ma proposition est d'éviter (dans la mesure du possible) les useEffects qui abstraient de la logique qu'on peut tout à fait contrôler directement via l'extérieur du composant. Et si on veut répondre à un besoin particulier via un useEffect pour facilier une abstraction, je préfèrerais qu'on le fasse par une prop explicite qu'on expose pour "activer" ce useEffect (ex: isUndefinedWhenDisabled sur les fields).

Ce que je proposerais donc ici c'est :

1. Retirer ce useEffect par défaut
2. • Soit recréer le composant depuis l'extérieur quand les props changent,
   • Soit ajouter une prop isReopenedOnIsClosableOrIsCollapsibleChange qui active ce useEffect. (pour éviter un saut visuel je suppose ?)

Qu'est-ce que vous en pensez ?

[sonarcloud]

Quality Gate failed

Failed conditions
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint