angular-inview icon indicating copy to clipboard operation
angular-inview copied to clipboard

Add `requireOffsetParent` option

Open evenicoulddoit opened this issue 8 years ago • 5 comments

Fixes #111

  • Add an optional requireOffsetParent option, which when applied only reports an element as in view when it also has an offsetParent

evenicoulddoit avatar Nov 11 '16 17:11 evenicoulddoit

amazing work with those tests!

It would be great if you could add documentation about that new option to the readme.

thenikso avatar Nov 17 '16 14:11 thenikso

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:

  • Are you happy with the 0-width 0-height solution merged in #116 as not being visible, if so I can remove the flag I've added here, remove some of my code, and restructure the tests to fit that more simplistic (& restrictive, depending on other's use cases) definition of "in view"
  • I might take a little more time to see if $timeout can be used more in both my additions and throughout the codebase generally
  • Will contribute to README depending on outcome of above

evenicoulddoit avatar Nov 17 '16 15:11 evenicoulddoit

Hmm 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?

thenikso avatar Nov 17 '16 16:11 thenikso

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).

lopsided avatar Nov 17 '16 17:11 lopsided

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.

dbrgn avatar Feb 28 '17 08:02 dbrgn