Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2ddf502
fix(components): make banner visible again when isClosable or isColla…
lwih Apr 24, 2024
8b32035
Merge branch 'main' into banner-fix
lwih Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/components/Banner/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,22 @@ function Banner({
const [isCollapsing, setIsCollapsing] = useState<boolean>(false)
const [hasCollapsed, setHasCollapsed] = useState<boolean>(false)

useEffect(() => {
// Reset visibility/height when isCollapsible or isClosable change
setIsHidden(false)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On ne re-init pas avec !!isHiddenByDefault ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ivangabriele ivangabriele May 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
    • 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 ?

setIsCollapsed(false)
setIsCollapsing(false)
setHasCollapsed(false)
}, [isCollapsible, isClosable])

const enterHover = (): void => {
if (!isHidden && isCollapsed && !isCollapsing) {
if (isCollapsible && !isHidden && isCollapsed && !isCollapsing) {
setIsCollapsed(false)
}
setIsCollapsing(false)
}
const leaveHover = (): void => {
if (!isHidden && hasCollapsed) {
if (isCollapsible && !isHidden && hasCollapsed) {
setIsCollapsed(true)
}
}
Expand Down
Loading