Merged
Conversation
hshoff
approved these changes
Dec 20, 2024
hshoff
reviewed
Dec 20, 2024
| observer.unobserve(curr); | ||
| } | ||
| }; | ||
| }, [ref]); |
Member
There was a problem hiding this comment.
Oh, might not want to use a ref as an effect dep. It'll be stable across re-renders and not trigger the effect. Can use useCallback(). https://legacy.reactjs.org/docs/hooks-faq.html#how-can-i-measure-a-dom-node
| }, []); | ||
|
|
||
| return { everVisible, ref }; | ||
| } |
Member
There was a problem hiding this comment.
What do you think about this hook instead?
function useIntersectionObserver() {
const [entry, setEntry] = useState<IntersectionObserverEntry | null>(null);
const prevObserver = useRef<IntersectionObserver | null>(null);
const ref = useCallback((node: Element) => {
if (prevObserver.current) {
prevObserver.current.disconnect();
prevObserver.current = null;
}
const observer = new IntersectionObserver(
([entry]) => {
setEntry(entry);
},
{ threshold: 0.01, root: null },
);
observer.observe(node);
prevObserver.current = observer;
}, []);
return [ref, entry];
}This could be a better approach for a few reasons:
- I think
useIntersectionObserveras a name feels like it more directly reflects its functionality, whileuseEverVisibleis more abstract. - I think using
disconnectis better because it completely removes the observer and stops watching all elements, ensuring proper cleanup, whereasunobserveonly stops watching a specific element without cleaning up the observer itself, which can lead to memory leaks if not handled properly. - We can return the full
IntersectionObserverEntry, giving access to more useful visibility data (maybe you don't want or care about this though since we're only using it to check if an element is visible or not). - We are only observing one element at a time in the hook, so even though the callback of the
IntersectionObserveralways returns an array of entries, the array will always have just one entry, so we don't need to loop over every entry. - The hook has fewer concerns about state tracking and effects.
Collaborator
Author
There was a problem hiding this comment.
I dig it! this is a great suggestion for all the reasons you listed, I was just being lazy (hah!) :)
Member
|
Ran this branch locally and it looked good! Merging even though it blanks out happo diff for wordcloud |
|
🎉 This PR is included in version |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🚀 Enhancements
Improve the performance of the initial gallery page load by lazy rendering visible tiles