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.

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

feat(Tooltip): fix dynamic width #778

Merged
merged 4 commits into from
Apr 11, 2024
Merged

feat(Tooltip): fix dynamic width #778

merged 4 commits into from
Apr 11, 2024

Conversation

alexbrillant
Copy link
Contributor

@alexbrillant alexbrillant commented Mar 22, 2024

DS-791

Ajouter une key prop au tooltip permet de forcer react à re-calculer le css dynamique avec les transform dans le component de la librairie react-popper-tooltip. Cette solution semble plus intéressante que d'overrider les styles de react-popper-tooltip.

Quand le label du tooltip change, on change la prop key et cela permet de mounter une nouvelle instance du tooltip et de re-calculer le css correctement pour le nouveau label. Cette amélioration garantira que le tooltip maintienne sa position indépendamment des changements de contenu, améliorant ainsi l'expérience utilisateur.

@alexbrillant alexbrillant requested a review from a team as a code owner March 22, 2024 02:07
@alexbrillant alexbrillant force-pushed the dev/DS-791 branch 3 times, most recently from 740a8b9 to b7d91cc Compare March 22, 2024 13:42
Copy link

Storybook for this build: https://ds.equisoft.io/pr-778/

@savutsang
Copy link
Contributor

savutsang commented Mar 22, 2024

Je ne connais pas react-popper-tooltip et son API a l'air tres limite. Cependant j'ai trouve ceci https://popper.js.org/docs/v2/lifecycle/#manual-update

Et cette methode est expose par react-tooltip-popper

const popperTooltip = usePopperTooltip(...)

popperTooltip.update(); 
// ou .forceUpdate()

Donc si on appelle ca quand on affiche le tooltip, ca devrait fixer le probleme. Si ca corrige, je prefere ca que le key.

const currentLabel = isClicked ? (confirmationLabel ?? label) : label;
const tooltipVariant = (mode === 'confirm' && isClicked) ? 'success' : 'normal';

useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ca va causer un double render. Tu peux utiliser un useRef+useMemo a la place du useState+useEffect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

c'est un peu ça le but. je veux trigger un re-render du tooltip pour que react popper tooltip re-calcul le css quand le texte change pendant qu'il est ouvert

Copy link
Contributor Author

Choose a reason for hiding this comment

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

    useEffect(() => {
        popperTooltip.update?.();
    }, [currentLabel]);

mieux avec update pas besoin de state et c'est plus explicite. mais il faut quand meme que j'utilise le useEffect pour forcer l'update seulement quand le text a changé

meriouma
meriouma previously approved these changes Apr 10, 2024
@alexbrillant alexbrillant enabled auto-merge (squash) April 11, 2024 19:00
@alexbrillant alexbrillant merged commit 78a7d61 into master Apr 11, 2024
20 checks passed
@alexbrillant alexbrillant deleted the dev/DS-791 branch April 11, 2024 19:09
mmorin-equisoft pushed a commit that referenced this pull request Apr 17, 2024
* feat(Tooltip): fix dynamic width

* feat(Tooltip): fix dynamic width

* feat(Tooltip): remove dynamic text story

* feat(Tooltip): update tooltip when label changes using useRef
pylafleur pushed a commit that referenced this pull request Jun 3, 2024
* feat(Tooltip): fix dynamic width

* feat(Tooltip): fix dynamic width

* feat(Tooltip): remove dynamic text story

* feat(Tooltip): update tooltip when label changes using useRef
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants