Skip to content
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

Open
wants to merge 2 commits into
base: next
Choose a base branch
from

Conversation

megvalcour
Copy link
Contributor

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.

Copy link

@tspears1 tspears1 left a 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

Copy link
Member

@khamer khamer left a 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 when isIntersecting 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
Copy link
Member

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)
Copy link
Member

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))
Copy link
Member

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not disconnect?

@khamer
Copy link
Member

khamer commented May 31, 2022

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.

@megvalcour
Copy link
Contributor Author

Thanks for the review, @khamer !

My thoughts on your thoughts:

  • Agreed 100% on having a concrete example to showcase how this is used. If I have time before I leave, I'm happy to add one in, otherwise it may be on you or @tspears1 (or anyone).
  • I disagree on not wanting to take a callback as an argument. In the Vue 3 docs, they create a "useEventListener" composable, and they take a callback as one of their arguments, plus around the web it seems to be a similar pattern -- so, I think it makes sense to keep to this pattern. Plus, I noticed in your playground implementation that you've added a watcher, which means 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, and then an action firing based on that. It seems simpler and more flexible for devs to just allow for a callback. Plus it gives more options to folks out of the box depending on their situation and preference.
  • Similarly, I disagree here on once -- I think in some instances, like lazyloading or entrance effects, you'll want it to fire once and then stop observing that element both performance and UX reasons. But for different, continual scroll effects like the one in your playground example, you'd want to continue observing the element until the component is unmounted. So, once gives the dev that option in an easy, simple way, but like with my point above, the flexibility to work as they'd like.

As always, @tspears1 , feel free to jump in if you disagree or want to add anything else!

@mbfitz
Copy link
Contributor

mbfitz commented Jun 2, 2022

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!

@khamer
Copy link
Member

khamer commented Jun 2, 2022

A better comparison would be useMouse() or useFetch() further down the same page in the docs.

By returning the reactive isIntersecting, you've already given everyone a way to run code when the value changes; callback and outCallback are redundant and shouldn't be required to construct useIntersectionObserver(). In a way, the value you're adding with UseIntersectionObserver is you're providing a reactive wrapper to something native that uses callbacks. The reactive wrapper shouldn't also use callbacks.

Most components should just use isIntersecting; something like

<div class="animate__animated" :class="{ animate__fadeInUp: isIntersecting }">

As far as as once - should once only fire once no matter what the change is? what should it do if the element is already visible? What if you want to have one animation run only the first time but another run every time?

Having a watcher only run once is something like

const unwatch = watch(isIntersecting, () => {
  ...
  unwatch()
})

@tspears1
Copy link

tspears1 commented Aug 2, 2022

@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?

@khamer
Copy link
Member

khamer commented Aug 3, 2022

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.

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.

4 participants