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

Popup appears in wrong spot when set manually before trigger is activated. #1065

Closed
keeslinp opened this issue Dec 20, 2016 · 13 comments
Closed

Comments

@keeslinp
Copy link
Contributor

Steps

  1. Set up a controlled popup(open prop).
  2. Set initial open state to true, or press an external button that can turn it on.
  3. Turn it on and off as much as you like, it'll stay in the top corner.
  4. Activate trigger (either press or hover depending what the "on" prop is)
  5. Now button makes popup appear in the correct spot.

Expected Result

The popup appears where it normally should.

Actual Result

It appears in the top right corner.

Version

0.63.0

Testcase

http://codepen.io/anon/pen/Mbxmyj

@keeslinp
Copy link
Contributor Author

I went through and I think I figured out what was wrong. Open isn't actually a prop of Popup, it is a prop of Portal and the example on the popup page takes advantage of the pass-through that Popup does. Popup relies on the event object in onOpen in order to find it's this.coords. My best guess would be that when the open prop is manually triggered there isn't an element to connect to that event so Popup gets lost.

I'm not entirely sure what the fix would be. A really sketchy fix I just did for myself is that I fired a click event on the trigger to help the Popup find it's trigger.
http://codepen.io/anon/pen/xRBrrN?editors=0011
Along the same lines another option would be to put the click in componentDidMount() and then take setState out of onOpen effectively disabling the trigger and forcing the popup to rely on the button.

A much better solution would be to find a way to get the trigger element without having to cause DOM events because that's not a very React thing to do.

@DuratarskeyK
Copy link

I have stumbled upon this issue as well. However in my case using synthetic events somehow wreaks havoc on the whole event system and some other unrelated events are fired. Please fix this.

@levithomason
Copy link
Member

It looks like the position is not being calculated at the correct time. PRs welcome.

@DuratarskeyK
Copy link

@levithomason I'm thinking of making popup stick to its trigger, because there are problems when I try to compute position when the trigger is rendered and then it changes position.

@levithomason
Copy link
Member

I think this makes sense, if the trigger moves, the popup should move with it.

@sdrothco
Copy link

I have added a PR for a possible fix for this here:
#1190

@stale
Copy link

stale bot commented Feb 4, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 30 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Feb 4, 2018
@nukeop
Copy link

nukeop commented Feb 13, 2018

I'm still seeing this, in my case I am controlling the popup from the outside by placing it inside a div container and opening it via the onContextMenu event of the div.

@stale stale bot removed the stale label Feb 13, 2018
@levithomason
Copy link
Member

Please see the linked PR #2389. Let's move any conversation on progress to there.

@JakobJingleheimer
Copy link

What was the reason Portal was used for this?

I think Popup should not be ripped out of the DOM and re-located. I believe this creates unnecessary complexity that could be solved more succinctly with basic css: Wrap the trigger in a container with a non-static (probably relative) position, and Popup's position set to absolute. This way, Popup will always be relative to its trigger. This would be more significantly more efficient on a number of levels.

I put together a very simple example: https://jsfiddle.net/jshado1/hzy4qvu9/

@levithomason
Copy link
Member

@jshado1 I assure you, no DOMs were ripped in the making of this Popup. We cannot make assumptions about the user's trigger styling. Forcing their trigger to position relative may have consequences.

Fixed in #2775

@djnorrisdev
Copy link

djnorrisdev commented Feb 24, 2019

I read #2775 and don't understand what the fix was. I'm still having this issue when using the open prop. When I remove the open prop and allow the popup to control it's own state, the hover is positioned correctly to the rendered container.

@levithomason
Copy link
Member

@djnorrisdev If there's a bug, let's open a new issue and we'll tackle it.

@Semantic-Org Semantic-Org locked as resolved and limited conversation to collaborators Feb 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants