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: popup position for point-aligned popups when no mouse position is known #447

Conversation

philippotto
Copy link
Contributor

When setting popupVisible of a <Trigger /> to true in a controlled manner, the popup is shown at 0,0 of the current page instead of being placed around its actual target. The reason for this is that the mouse position is initialized to 0, 0 and then passed along to useAlign.
This is a regression that was introduced in 848a197. In 7590937 this used to work and it had the exact logic I now re-added.

Within 848a197, the missing null initialization was introduced here:
848a197#diff-0b5adbfe7b36e4ae2f479291e20152e33e940f7f265162d77f40f6bdb5da7405R310
And here's the conditional passing of the point that was removed: 848a197#diff-0b5adbfe7b36e4ae2f479291e20152e33e940f7f265162d77f40f6bdb5da7405L542

As a test, I set popupVisible to true in rc-trigger/docs/examples/point.tsx so that the popup is rendered by default.

Before:
image

After:
image

Would fix ant-design/ant-design#47152.

… still placed at its target when alignPoint is true, but no mouse position is known
src/index.tsx Outdated Show resolved Hide resolved
@afc163
Copy link
Member

afc163 commented Mar 11, 2024

Could you add a test case ?

Copy link

codecov bot commented Mar 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.68%. Comparing base (50ae862) to head (4e66204).
Report is 16 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #447   +/-   ##
=======================================
  Coverage   97.68%   97.68%           
=======================================
  Files          13       13           
  Lines         776      776           
  Branches      221      221           
=======================================
  Hits          758      758           
  Misses         18       18           

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

@philippotto
Copy link
Contributor Author

Could you add a test case ?

I tried, but apparently I'm doing something wrong. You can have a look at this commit: scalableminds@74b13aa

Quoting from my comment in that commit:

      // This test does not work at all at the moment, because useAlign calls
      // isVisible on the trigger's target (div with class "point-region").
      // For that div, getBoundingClientRect() always returns a width and height
      // of zero. Therefore, it is interpreted as not visible and useAlign
      // returns ready==false.

If you know an easy fix for this, I can incorporate it. However, I don't have much time diving into this further. Maybe the PR is alright without a test?

@afc163 afc163 changed the title Fix popup position for point-aligned popups when no mouse position is known fix: popup position for point-aligned popups when no mouse position is known Sep 3, 2024
@afc163 afc163 merged commit 13bfb83 into react-component:master Sep 3, 2024
9 checks passed
zombieJ pushed a commit that referenced this pull request Sep 3, 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.

Controlled Dropdown component rendered at wrong position
2 participants