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

Update isVisible #178

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

SalvoDiFede
Copy link

Using box.getBoundingClientRect() for better compatibility, please check and test before commit to main branch :-)

Using box.getBoundingClientRect() for better compatibility, please check and test before commit to main branch :-)
@attilaolah
Copy link
Collaborator

This looks very cool, but apparently the tests don't like it. I'll dig into that and see why.

Maybe we can also kill that funky offsetTop method then.

@attilaolah attilaolah self-assigned this Apr 15, 2015
@jlmakes
Copy link

jlmakes commented Dec 20, 2015

I tried implementing element.getBoundingClientRect() into ScrollReveal v3.0.0, and I can confirm it's likely not suitable for our scrolling animation libraries—the visibility factors in CSS transformations, whereas element offset does not.

This is most noticeable with large translations… where in some cases, the element will either be visible/invisible at the same time (based on the time at which is checked during animation), or never be visible at all.

A simple example can be seen here ScrollReveal Issue #193

Hope this helps! 🙇

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.

3 participants