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

Fix popover positioning and optimize rendering logic #202

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cozmo
Copy link

@cozmo cozmo commented Oct 4, 2024

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
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 (
    <div style={{ display: "flex", justifyContent: "center", alignItems: "center", height: "100vh" }}>
      <Popover
        isOpen={isPopoverOpen}
        positions={["top"]}
        content={({ position, childRect, popoverRect }) => (
          <ArrowContainer
            position={position}
            childRect={childRect}
            popoverRect={popoverRect}
            arrowColor={"black"}
            arrowSize={12}
          >
            <div
              style={{
                padding: "20px",
                backgroundColor: "black",
                color: "white",
                maxWidth: isLargeContent ? "600px" : "200px",
                minWidth: isLargeContent ? "400px" : "100px",
                wordWrap: "break-word",
                fontSize: isLargeContent ? "16px" : "14px",
                lineHeight: isLargeContent ? "1.6" : "1.2",
              }}
            >
              {popoverContent}
            </div>
          </ArrowContainer>
        )}
      >
        <div
          style={{
            padding: "10px 20px",
            border: "1px solid black",
            cursor: "pointer"
          }}
          onMouseEnter={() => {
            setPopoverContent(generateRandomContent(isLargeContent));
            setIsPopoverOpen(true);
            setIsLargeContent(!isLargeContent);
          }}
          onMouseLeave={() => {
            setIsPopoverOpen(false);
          }}
        >
          Hover Me to Toggle Popover
        </div>
      </Popover>
    </div>
  );
}

const container = document.getElementById("root");
if (container) {
  createRoot(container).render(<App />);
}

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.

cozmo added a commit to cozmo/react-tiny-popover that referenced this pull request Oct 4, 2024
@cozmo cozmo force-pushed the cleanup-positioning-code-to-avoid-incorrect-position-flash branch from 5359bd4 to 22927b2 Compare October 5, 2024 17:54
- Eliminate flash of incorrect position on popover open
- Simplify useLayoutEffect hook for more clean and correct positioning calculation
- Adjust event listener management in useEffect to accomplish the same
@cozmo cozmo force-pushed the cleanup-positioning-code-to-avoid-incorrect-position-flash branch from 22927b2 to c7aef7c Compare October 5, 2024 17:55
cozmo added a commit to cozmo/react-tiny-popover that referenced this pull request Oct 5, 2024
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.

1 participant