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

Fixed a bug for TooltipControl.js where the tooltip would not render correctly #1407

Closed
wants to merge 1 commit into from

Conversation

dogoku
Copy link
Contributor

@dogoku dogoku commented Jun 1, 2015

It is expected behaviour that if the element that the tooltip needs to attach itself to is not visible, the tooltip will not render at all.

But if the tooltip needs to visible immediately when the page loads and the parent element is not visible yet, the tooltip will not render.

For that reason, we need to delay execution until rendering is complete

This is easy to test in CT. Please talk to me for a url

…correctly

It is expected behaviour that if the element that the tooltip needs to attach itself to is not visible, the tooltip will not render at all.
But if the tooltip needs to visible immediately when the page loads and the parent element is not visible yet, the tooltip will not render

For that reason, we need to delay execution until rendering is complete
@andy-berry-dev
Copy link
Member

Hey @dogoku, thanks for the PR. Could you also write a test to go with the change? Thanks!

@andy-berry-dev andy-berry-dev modified the milestones: 1.1, Post 1.0, 1.0 Jun 1, 2015
@dchambers
Copy link
Contributor

@andyberry88, apparently this change can only be seen in CT, and is not easy to replicate in BRJS, so I was inclined to allow it through provided it could be manually tested with CT. However, I've just looked at the code and seen that the fix is to use a setTimeout(), which just means that we are swapping an easy to reproduce bug for a hard to reproduce bug, whereas I'd far prefer to have an easy to reproduce bug.

@dogoku, can't your code wait on some underlying event, and in fact wouldn't that then make it more testable?

@dchambers
Copy link
Contributor

I'm pretty sure this has already been fixed by #1378.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants