Issue 414

[piotr-gawron]

This PR fixes issue #414. However, there are problems with tests now. Tests works perfectly fine when
they are run in a browser, but when test are run in a console there are errors. The errors can be summarized as: object 'document' has no function 'contains'.

Can you help me with tests?

Summary of my PR: when pileup is created with 'element' that doesn't belong to DOM, observer object is created. This observer observes document object. When 'element' is added to document (or to any (grand)child) of document pileup tracks are forced to rerender by changing updateSize state of ReactElements.

Sorry for the commits comments but I have problems with lint and git in general ;/

---

This change is

[armish]

TIL that document.contains is not a standard thing: http://stackoverflow.com/questions/20557115/mocha-casperjs-headless-testing-document-contains-undefined

but if (!document.body.contains(el)) { works fine as suggested in that SO answer. This should fix the issue with the headless tests.

[piotr-gawron]

I dissagree with this answer at SO. Here you have better source:
https://developer.mozilla.org/en-US/docs/Web/API/Node/contains

It is a standard, the only remark is that IE doesn't support it for document but only for document.body.

I can change it to document.body if it solves the problem.

Anyway, I took it from this answer at SO:
http://stackoverflow.com/questions/5629684/how-to-check-if-element-exists-in-the-visible-dom/5629730#5629730

[armish]

as an alternative, you can do:

var contains = document.contains || document.body.contains;
if (!contains(el)) {
...
}

PhantomJS framework can sometimes surface issues like this; but, the pros still outweigh its cons, so we are still sticking to it. Also, any such issue will affect people trying to render views in headless mode, so it would be great if we address this even though it is a weird workaround.

[piotr-gawron]

this doesn't work: I get some strange Invalid invocation error in Chrome, Firefox reports different error...

But I fixed it by replacing: document.contains with document.body.contains

[armish]

thanks @piotr-gawron! See my inline comments/questions regarding some functionality and performance issues.

It would also be great to be able to replicate this issue within a unit test, but I do understand it might be tricky/unfeasible to do so. But having such as a case might help us do some benchmarking and performance monitoring.

On a side note: do you think it would be better if we expose an updateSize method within the main pileup object and document this edge case in case somebody else hits a similar issue? That way, we don't have to think about how this might affect other users in a broader sense.

[coveralls]

Coverage decreased (-0.6%) to 91.081% when pulling 04d0da5 on piotr-gawron:issue-414 into ed543e6 on hammerlab:master.

[piotr-gawron]

@armish
Sorry for long answer - it's vacations time and I wasn't available for the last 2 weeks.

[coveralls]

Coverage decreased (-0.6%) to 91.071% when pulling 8480090 on piotr-gawron:issue-414 into ed543e6 on hammerlab:master.

[piotr-gawron]

@armish I created this PR almost a year ago. Do you plan to merge it? Or there are some issues with my code?

[armish]

hey @piotr-gawron - sorry that this fell through the cracks and I wasn't able to get back to you with my second round of reviews. I originally wanted to do some style/type cleaning up on your code before merging in, but for the last few months, I barely have the additional time to channel into pileup maintenance.

Let me just take a quick look at your PRs and I will merge them in if there are no apparent red flags.

Apologies for the delay.