Open evenicoulddoit opened 8 years ago
amazing work with those tests!
It would be great if you could add documentation about that new option to the readme.
Hey, sure, happy to add to README.
BTW #116 was a more trivial way of determining whether an element is within the viewport, but I opted against it because I didn't want to assume that people were never using inview with 0-width, 0-height elements.
If you are happy with that restriction. then any element within a modal/tab or similar should also be picked up by that commit (though I haven't checked to be sure).
That would make the tests I've written here more broadly application and you wouldn't need the flag at all.
The only other thing I have applied is a timeout, to allow for a digest cycle after clicking. I've not looked more into why you're not using $timeout
, but I could take another look.
In summary:
$timeout
can be used more in both my additions and throughout the codebase generallyHmm I merged #116 too quickly then. I can in fact see use cases where 0-height elements might be useful to check for inview (ie: sentinels).
I now understand the need of basically "waiting for initial render" before checking inview.
However I think that a better, more retro-compatible approach might be to add those special cases as additional options (see ie generateDirections
).
In #116 the problem to be addressed isn't the 0-width/height element being checked for inview early but rather that that element will then change size and yet the inview event is not fired anymore (correct?). If that's the case I'd see (let's say) an triggerOnSizeChange
option that, when set to true, would re-trigger the inview callback if the element size changed but inview value didn't.
For this PR instead, I still can't understand when an element which should have an offset parent could be checked for inview but not have an offset parent just yet. It seams like a but where we are not waiting for a dom ready event first (which should be addressed by angular).
am I making sense?
I agree the 0-height could be useful for some applications, that's why I didn't create the PR initially and only made an issue. However, in the fiddle I provided there is no change in height for the element that has the in-view
directive on it, but other elements on the page. I don't think it would be feasible to add a triggerOnSizeChange
to all the other elements on a site that could potentially change size - certainly not on my current project anyway.
Incidentally @thenikso , you mentioned in #113 that you could fix the fiddle by changing the ng-show
to ng-if
. I tested this but it was still doing the same thing (try showing and hiding a few times, then it gets stuck saying it is "in view" even when it is both off the screen and also hidden! (I thought I'd reply to that here to keep this to just one discussion thread).
This PR looks like it would do the trick of detecting "display: none" and so returning "not-in-view" when that is the case. However offsetParent
will also return null
for position: fixed
. Not sure if that matters or not.
Obviously I can't vouch for everyone using this plugin, but I tend to think that "in-view" should default to this behaviour (ie, returning false when it is not in the viewport or not visible). So instead adding an option like require-visible
which defaults to true and allow users to include 0-dimension elements might make more sense (to me at least).
Nice work with the tests in deed. But does it really solve #111?
If I run the following script in the browser console on Github:
setInterval(() => console.log(document.querySelector('h1').offsetParent), 1000)
...then it reports an offsetParent element even when using a different tab in Chromium.
Fixes #111
requireOffsetParent
option, which when applied only reports an element as in view when it also has an offsetParent