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

Add requireOffsetParent option #119

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

evenicoulddoit
Copy link

Fixes #111

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

@thenikso
Copy link
Owner

amazing work with those tests!

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

@evenicoulddoit
Copy link
Author

evenicoulddoit commented Nov 17, 2016

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 Check if element is visible and if not return false for "inView" #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

@thenikso
Copy link
Owner

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?

@lopsided
Copy link
Contributor

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

@dbrgn
Copy link

dbrgn commented Feb 28, 2017

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.

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

Successfully merging this pull request may close these issues.

4 participants