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

Allow setting rootMargin #486

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

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Oct 26, 2021

Part of #428.

The following tasks have been completed:

  • Modified Web platform tests (link to pull request)

Implementation commitment:


Preview | Diff

@zcorpan zcorpan requested a review from emilio October 26, 2021 22:03
@zcorpan zcorpan mentioned this pull request Oct 26, 2021
4 tasks
zcorpan added a commit to bocoup/IntersectionObserver that referenced this pull request Oct 26, 2021
@zcorpan zcorpan changed the title Allow setting rootMargin Allow setting rootMargin Oct 26, 2021
Copy link
Collaborator

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks reasonable, but of course needs tests and bugs and so on.

@szager-chromium
Copy link
Collaborator

What effect, if any, will modifying rootMargin have on previousThresholdIndex for active observations? Perhaps they should all have previousThresholdIndex set to -1, just as it is when observe() is first called:

https://w3c.github.io/IntersectionObserver/#observe-target-element

One side effect would be that modifying rootMargin would cause every observation to generate a notification at the next opportunity.

@ampedweb
Copy link

ampedweb commented Sep 9, 2024

Hey guys, I'm curious how far off this is? I found myself here after looking for a way to update rootMargin after instantiation, this would pretty much solve my issue 😄.

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