Fix for Tooltip position in Firefox on SVG's

[psaia]

Possibly related to #99

The offsetWidth and offsetHeight were coming in undefined thus resulting in a calculated NaN. Resolving them to 0 did the trick.

[jquense]

looks good. please squash your commits and follow the commit guidelines here: https://github.com/react-bootstrap/react-bootstrap/blob/master/CONTRIBUTING.md and we are good to go. thanks for helping out :)

[AlexKVal]

Do we need some test to prevent future regressions ?

[taion]

Ideally, but in this case I'd be okay with adding a comment unless you want to set up Travis to also use Firefox.

[AlexKVal]

I've tried Firefox tests.
There are a lot of work to do first. Many tests are failing.
At least it was true 1.5 months ago.
It is on my ToDo list, buy with a low priority.

Then I'm fine with just a comment 😄

But a squashing is definitely needed 😓

[taion]

Okay. @petesaia: can you add a comment explaining why this is necessary (maybe with a TODO to add a test case to cover this), and squash your commits? We'll deal with setting up CI on FF later.

[psaia]

So after looking at this issue again with a clearer head, I'm a little confused. The problem is happening because offsetWidth and offsetHeight are assumed to always be available on an element. A way of fixing this would be to use getBoundingClientRect().

With that said, it looks like getOffset() and getPosition() are doing this properly (unless jquery exists). They are also being called within the same function(s), yet offsetWidth and offsetHeight are still being used after the fact. Why not just use the values from getOffset() and getPosition()? This might be a good time to remove jquery.

I squashed my commits, but i'd just recommend using the values from these methods so logic is more centralized. What do you think?

Update: More to the point - I don't think there is any real problem with using jQuery, I just think the values from these methods should be more consistent with getBoundingClientRect()'s return values: left, top, right, bottom, width, height

[taion]

We use jQuery for IE8 compat - same reason I don't use GBCR there, because it doesn't return width/height on IE8.

[taion]

@petesaia Can you add the comment and squash your commits?

[psaia]

@taion I hate to sound so noobish, but could you explain how else you'd like me to squash the commits and where to comment? So far, i've done a git rebase -i HEAD~4 for my four commits and merged (6e476fe). As detailed as CONTRIBUTING.md is, I don't see anything about this, specifically. Sorry!

[taion]

Don't do the merge yourself - when I click the "merge" button, GH will do it for me.

What you need to do is to do git rebase -i master and then choose s or f for all the commits after the first, fix the commit comment, then do git push -f.

Add the comment where you made your changes.

[psaia]

psaia@e9858ce

Sorry for the delay!

[taion]

No worries. Looking good. Can you just add that comment around there real quick explaining why this is necessary?

[jquense]

@petesaia I think he means add a comment to the code itself, not just on the PR diff.

[psaia]

Done.

[taion]

Gahhh merge conflict. Mind rebasing and re-submitting? This looks good to go otherwise.

[psaia]

I've rebased, but it's still throwing a:

Can’t automatically merge. Don’t worry, you can still create the pull request.

Any ideas?

[taion]

You probably need to do: https://help.github.com/articles/syncing-a-fork/

[psaia]

Sent a new one.

[mtscout6]

For future reference as well if you don't want to close a PR just to open a new one you can use git to force push your branch after a rebase. That will update the PR with the new branch automatically. Force push can be problematic in some cases which is why some people are weary of it, but it is useful in cases like this.

[psaia]

@mtscout6 That's certainly good to know, thanks.