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 Repositioning of Tooltips on Resize #21

Closed
jmrog opened this issue Oct 24, 2015 · 3 comments
Closed

Fix Repositioning of Tooltips on Resize #21

jmrog opened this issue Oct 24, 2015 · 3 comments

Comments

@jmrog
Copy link
Contributor

jmrog commented Oct 24, 2015

@gilbarbara, in my PR #20, I mentioned that I was having some issues with positioning of elements in the joyride. After doing some investigating, it seems that the check at line 193 in the most recent version (https://github.com/gilbarbara/react-joyride/blob/master/lib/scripts/Mixin.js#L193) prevents visible tooltips from updating their position when the window is resized. As far as I can tell, the check that prevents this was initially added in commit 9f932e9 ("fix autorun"). Whenever the tooltip has an X position that is greater than -1, it gets the animate class via props. But calcPlacement does not reposition any element that both has the animate class and has an X position greater than -1, with the result that tooltips never reposition themselves.

Changing line 193 so that it only checks if step exists (which is what it used to do before 9f932e9) seems to fix this issue. Did you make the change in 9f932e9 in order to prevent updating state too frequently on resizing?

@jmrog
Copy link
Contributor Author

jmrog commented Oct 24, 2015

Just to add: if you go to your demo, open a tooltip, and then resize the window, you can see the behavior I'm talking about (no repositioning -- the tooltip just "hangs"). The demo works with the changes from PR #22 incorporated, though.

If the PR is merged, an additional option to consider is allowing the user to adjust the sensitivity of the debouncing. I initially had it at 100ms, but that was still too sensitive -- updates were too frequent -- so I changed it to 200ms, but right now it is hard-coded.

@gilbarbara
Copy link
Owner

hey @jmrog
Yeah, I've added those checks to prevent too many state changes but I forgot to test the resizing afterwards..
I've copied your changes locally and will test some more.

@gilbarbara
Copy link
Owner

@jmrog Pushed 0.7.4 to npm
Thanks

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

No branches or pull requests

2 participants