-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Custom hook using IntersectionObserver #35835
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please also verify this works for the Timeago
component?
Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>
quick comment before a full review: given that the win argument is derivable from the ref (first parameter), can we skip providing it and instead have the hook derive it? |
@samouri thanks for the comment - it's one of the open questions (mentioned in the description) we'd like input on. The scenario we thought for allowing a user to specify the |
Co-authored-by: Jake Fried <samouri@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also create unit tests for this hook?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, had a saved comment that I had never hit submit on 😬
Co-authored-by: Jake Fried <samouri@users.noreply.github.com>
* @return {function(!Element)} | ||
*/ | ||
export function refs(...refs) { | ||
return (element) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use useCallback
to avoid trashing the ref.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cant use useCallback
outside a component or hook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make it a hook, then. As is, this is going to call each refs
every time it renders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pretty slick* implementation: https://github.com/theKashey/use-callback-ref#usemergerefs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merged this PR as it has too many comments as it is. Created #36024 for the custom hook implementation.
This PR creates the
useIntersectionObserver
custom hook which calls a given callback on a given element with the most recentIntersectionObserverEntry
received with where that element is a target. A consumer could use it as follows:The current approach is preferred to the following because it may cause extraneous intermediary renders in the consumer:
Note the above also has bugs when multiple targets are being observed - the update
lastEntry
state by the hook would be incorrectly shared by all consumers.Open questions:
IntersectionObserver
per target window instance, or can a globalwindow
suffice? The current approach is to take an optional target window and use the local window relative to the givenref
.IntersectionObserver
per initoptions
? This can grow rather quickly if there are specific use cases for certain thresholds, but as things are, the only two applications (Timeago
andIframe
) do not specifyoptions
, so perhaps it's better to support this case once the need arises rather than over-engineer on a hypothetical.