Skip to content

Issue #12 suggestion - add root and visibleOffset to useEffect deps #25

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

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

Conversation

pklingman
Copy link

Issue: #12

This is a great library! I think this PR would be helpful for one of our use cases for two main reasons, and also adding these props to the dependencies array follows the rule of hooks:

  1. Currently root must be defined on the first render or it will always be null - this allows root to be defined on subsequent renders. Using an element that is not available in the DOM on the first render is something we are currently doing.
  2. If root is null, then rootMargin in the intersection observer library is not applied

@zzynx
Copy link

zzynx commented Nov 29, 2023

Good fix.
I made my own MyRenderIfVisble component by copying the code of the current render-if-visible.tsx and applied your changes to it. That works fine.

Btw,
eslint also suggested me to add those dependencies.
So indeed, adding these props to the dependencies array follows the rule of hooks.

@VenyaBrodetskiy
Copy link

@pklingman I had to make same changes for my project for same reason.
Could you please @GusRuss89 approve and merge this PR? Currently I can't use your package in my project, this PR would fix the issue.
Thank you!

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