-
Notifications
You must be signed in to change notification settings - Fork 10
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
Initial commit of useIntersectionObserver composable. #70
base: next
Are you sure you want to change the base?
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.
I really like outputting the Observer at the end. This way the user has access to the other options on each entry like entry.intersectionRatio
and entry.intersectionRect
. Looks good to me
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.
Overarching thoughts:
- We should include an example .vue component that makes use of this and has its own .twig file so we can see it at work in the browser.
- I don't think this should take callbacks as arguments. When returning reactive objects (like
isIntersecting
) devs using this composable should hook onto that instead. - Similarly,
once
feels awkward here. Deciding whether I want to do something whenisIntersecting
first becomes true or false or every time doesn't seem like something UseIntersectionObserver.js should worry about.
* @param {HTMLElement} elementToWatch elementToWatch | ||
* @param {function} callback callback once element is intersecting | ||
* @param {function} outCallback callback once element is not intersecting (optional) | ||
* @param {Boolen} once if callback only run one time |
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.
Boolean
} | ||
) { | ||
const isIntersecting = ref(false) | ||
const observer = ref(null) |
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.
observer
shouldn't be a reference - its value never changes and you're not expecting/handling changes to its value.
}, options) | ||
|
||
// Observe/unobserve within lifecycle hooks | ||
onMounted(() => observer.value.observe(elementToWatch)) |
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.
They recommend wrapping using variables passed into composables with unref()
so that your code will handle if someone passes in either a ref or a value.
|
||
// If the callback should only run once, unobserve the element | ||
if (once) { | ||
observer.value.unobserve(entry.target) |
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.
Why not disconnect?
Here is a refactor I did while just working through the review, in case it helps at all. There's more we could clean up/refactor here too, but just an example of what I'm suggesting. |
Thanks for the review, @khamer ! My thoughts on your thoughts:
As always, @tspears1 , feel free to jump in if you disagree or want to add anything else! |
First - yes, let's definitely get a concrete example of this working in BP. If Megan can't get to it before she leaves, and Tim is under water maybe I can give it a go when I have time. Second - I have to agree with Megan's 2nd and 3rd points here. Seems like the standard is using callbacks so we should go that route, and the "once" is definitely handy and a lot of libs offer an option like that for different scenarios, too. And lastly let's just appreciate this gem "instead of just having an intersection observer observing the element and calling the callback(s) as needed, you have the observer observing and then a watcher watching the results of the observer". I think next steps are to get a working example up and merge it in! |
A better comparison would be useMouse() or useFetch() further down the same page in the docs. By returning the reactive Most components should just use
As far as as Having a watcher only run once is something like
|
@khamer @TristanMNorton okay looked at a few examples of this in the wild and thought about how to make it hopefully the most useful. The current setup that Megan and I proposed I think is helpful but its almost too restrictive and I now think unnecessary. Here's my new thinking: demo This still allows the engineer full access to all of the tools inside of IntersectionObserver and makes it work well with Vue 3 (that unref is key for sure). Its very easy if you're just trying to see if something is intersecting or not OR you do something more complicated if you needed to. The example above would be one pattern to use if you were still after the ONCE option we first laid out. Thoughts? |
Sure, heading the right direction - the next thing I'd think about is whether we want this composable to work with a single or multiple targets. I'd guess optimizing for a single target makes the most sense, and given that, we can probably refactor to completely avoid passing a callback. If you want it to support multiple targets - which seems overkill for this to me - then maybe a callback makes sense, but we need to change how the composable is used so you can pass in multiple elements. |
Per https://workshop.imarc.com/projects/1486/tasks/216631, @tspears1 and I created a composable that leverages the Intersection Observer API and the onMounted/onUnMounted Vue hooks.
Import it into your Vue components so you can easily execute a callback (animation, lazyloading, pulling data from an API, etc.) based on an element entering or exiting the viewport.