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

Recalculate popover position when resized while open #1855

Merged
merged 1 commit into from
Jan 20, 2025

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Jan 20, 2025

This PR fixes an incorrect popover positioning when it gets resized or its contents change while it is open, as the position is calculated only when opened.

With this, we add a ResizeObserver which observes the popover, and if it gets resized, we re-calculate its position.

The incorrect position usually happens only if the popover had to pop up.

This is how it looks before the fix, when the contents of the popover change after it has been opened:

resize-calculate-2025-01-20_11.54.40.mp4

And this is how it looks once fixed:

resize-calculate-fixed-2025-01-20_11.55.11.mp4

There's a bit of flickering in the screen recordings above. I think that's an issue on the video recording software, as I don't see that on my screen.

The first time I notice this issue was while working in at-mentions. The suggestions popover content can change while it's open, causing the position to be wrong.

Test steps

  1. Apply this diff:
    diff --git a/src/pattern-library/examples/popover-basic.tsx b/src/pattern-library/examples/popover-basic.tsx
    index e44656c..acc0092 100644
    --- a/src/pattern-library/examples/popover-basic.tsx
    +++ b/src/pattern-library/examples/popover-basic.tsx
    @@ -1,4 +1,4 @@
    -import { useRef, useState } from 'preact/hooks';
    +import { useCallback, useEffect, useRef, useState } from 'preact/hooks';
     
     import { Popover } from '../../components/feedback';
     import { Button } from '../../components/input';
    @@ -6,6 +6,27 @@ import { Button } from '../../components/input';
     export default function App() {
       const [open, setOpen] = useState(false);
       const buttonRef = useRef<HTMLButtonElement | null>(null);
    +  const [showSecondParagraph, setShowSecondParagraph] = useState(true);
    +  const timeoutRef = useRef<ReturnType<typeof setTimeout>>();
    +  const cancelTimeout = useCallback(() => {
    +    if (timeoutRef.current) {
    +      clearTimeout(timeoutRef.current);
    +    }
    +    timeoutRef.current = undefined;
    +  }, []);
    +
    +  useEffect(() => {
    +    if (open) {
    +      timeoutRef.current = setTimeout(() => {
    +        cancelTimeout();
    +        setShowSecondParagraph(false);
    +      }, 2000);
    +    } else {
    +      setShowSecondParagraph(true);
    +    }
    +
    +    return cancelTimeout;
    +  }, [cancelTimeout, open]);
     
       return (
         <div className="relative flex justify-center">
    @@ -22,7 +43,8 @@ export default function App() {
             anchorElementRef={buttonRef}
             classes="p-2"
           >
    -        The content of the popover goes here
    +        <p>The content of the popover goes here</p>
    +        {showSecondParagraph && <p>Disappears in 2s</p>}
           </Popover>
         </div>
       );
  2. Go to http://localhost:4001/feedback-popover
  3. Scroll or resize the window so that the first "Open popover" button is close to the bottom of the viewport. That will cause the popover to pop-up.
  4. Open the popover and wait for 2 seconds for part of its content to disappear. In this branch, the popover should reposition to keep stuck on top of the button. In `main branch, it will keep its previous position, increasing the distance with the top of the button.

Copy link

codecov bot commented Jan 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (0255544) to head (2ccac98).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #1855   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           70        70           
  Lines         1303      1306    +3     
  Branches       482       482           
=========================================
+ Hits          1303      1306    +3     

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

@acelaya acelaya force-pushed the open-popover-resize branch from 351c238 to 8382061 Compare January 20, 2025 11:23
@acelaya acelaya force-pushed the open-popover-resize branch from 8382061 to 2ccac98 Compare January 20, 2025 11:57
@@ -151,6 +165,42 @@ describe('Popover', () => {
});
});

[true, false].forEach(asNativePopover => {
it('repositions popover if it is resized after being open', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test fails without the observer logic.

@acelaya acelaya marked this pull request as ready for review January 20, 2025 11:58
@acelaya acelaya requested a review from robertknight January 20, 2025 11:58
Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -133,9 +133,9 @@ function usePopoverPositioning(

// First of all, open popover if it's using the native API, otherwise its
// size is 0x0 and positioning calculations won't work.
const popover = popoverRef.current;
const popover = popoverRef.current!;
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a local hook I think it is OK to rely on the caller to ensure that popoverRef.current is set if popoverOpen is true. If this hook was exposed for consumers of the crate to use, I think we'd need to be more defensive.

@acelaya acelaya merged commit 629e59c into main Jan 20, 2025
4 checks passed
@acelaya acelaya deleted the open-popover-resize branch January 20, 2025 12:22
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.

2 participants