alexkatz / react-tiny-popover

A simple and highly customizable popover react higher order component with no other dependencies! Typescript friendly.
MIT License
454 stars 121 forks source link

Fix popover positioning and optimize rendering logic #202

Open cozmo opened 1 month ago

cozmo commented 1 month ago

When using the ArrowContainer component, a bug in the underlying Popover component is displayed, specifically, in how the popoverRect data is calculated. When opening a popover, the popoverRect is briefly set (and subsequently, passed to the content(..) function, as the prior value. This causes the arrow to visibly flash as it updates from the prior to the next value of popoverRect. bug

The code to reproduce this bug is relatively simple, and I provide it below, but the only real trick is changing the size of the popoverRect (otherwise it'll only appear on first render).

Code to reproduce the above gif ```tsx import React, { useState, useCallback } from "react"; import { createRoot } from "react-dom/client"; import { Popover } from "./src/Popover"; import { ArrowContainer } from "./src/ArrowContainer"; function App() { const [isPopoverOpen, setIsPopoverOpen] = useState(false); const [popoverContent, setPopoverContent] = useState(""); const [isLargeContent, setIsLargeContent] = useState(true); const generateRandomContent = useCallback((isLarge) => { const words = { short: ["Hi", "Hello", "Hey"], long: ["Extraordinary", "Phenomenal", "Unbelievable", "Fascinating", "Remarkable", "Astonishing", "Magnificent", "Spectacular"] }; const randomWords = (list, count) => Array.from({ length: count }, () => list[Math.floor(Math.random() * list.length)]).join(" "); if (isLarge) { const sentenceCount = Math.random() * 3 + 1; // 3 to 5 sentences return randomWords(words.long, Math.random() * 10 + 15) + ".".repeat(sentenceCount); } else { return randomWords(words.short, Math.random() * 3 + 2) + "!"; } }, []); return (
(
{popoverContent}
)} >
{ setPopoverContent(generateRandomContent(isLargeContent)); setIsPopoverOpen(true); setIsLargeContent(!isLargeContent); }} onMouseLeave={() => { setIsPopoverOpen(false); }} > Hover Me to Toggle Popover
); } const container = document.getElementById("root"); if (container) { createRoot(container).render(); } ```

This PR fixes this issue by simplifying and cleaning up the logic used in the useLayoutEffect hook, and generally the logic used to compute the layout, specifically by simplifying the useLayoutEffect hook, removing any use of animation frames and rather just directly checking for changes in relevant properties and call positionPopover when needed.

These changes resolve the flickering issue with the arrow while also simplifying the logic to be less prone to edge cases like this.

The same code as above with the fixes:

fixed

Also confirmed things continue to work when resizing the window. Generally this seems like a safe/good change to me, and was fixing the issues I was hitting, but if there are hidden issues with this let me know and I'm happy to iterate/add some documentation to this code, since it's fairly gnarly right now.