Conversation
caroqliu
left a comment
There was a problem hiding this comment.
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>
samouri
left a comment
There was a problem hiding this comment.
Can we also create unit tests for this hook?
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.
Use useCallback to avoid trashing the ref.
There was a problem hiding this comment.
Cant use useCallback outside a component or hook.
There was a problem hiding this comment.
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.
This is a pretty slick* implementation: https://github.com/theKashey/use-callback-ref#usemergerefs
There was a problem hiding this comment.
Merged this PR as it has too many comments as it is. Created #36024 for the custom hook implementation.
This PR creates the
useIntersectionObservercustom hook which calls a given callback on a given element with the most recentIntersectionObserverEntryreceived 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
lastEntrystate by the hook would be incorrectly shared by all consumers.Open questions:
IntersectionObserverper target window instance, or can a globalwindowsuffice? The current approach is to take an optional target window and use the local window relative to the givenref.IntersectionObserverper initoptions? This can grow rather quickly if there are specific use cases for certain thresholds, but as things are, the only two applications (TimeagoandIframe) do not specifyoptions, so perhaps it's better to support this case once the need arises rather than over-engineer on a hypothetical.