mediamonks / react-kit

A collection of React hooks, components and utilities we use at Media.Monks
https://mediamonks.github.io/react-kit/
MIT License
9 stars 2 forks source link

Pinned scroll animations break when dependencies change #341

Open larsvanbraam opened 4 months ago

larsvanbraam commented 4 months ago

Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch @mediamonks/react-kit@2.0.4 for the project I'm working on.

So in my project (next) I used the useMediaQuery hook in combination with the useScrollAnimation hook to do a have a custom scroll trigger animation per screen size.

const isLargeViewport = useMediaQuery(mediaQueries.LARGE, false);

useScrollAnimation(
  () => createScrollInAnimation(refs, isLargeViewport),
  [refs, isLargeViewport],
);
export function createScrollInAnimation(
  refs: SomeComponentRefs,
  isLargeViewport: boolean,
): gsap.core.Animation | undefined {
  const [isValid, validatedRefs] = validateAndUnwrapRefs(refs);

  if (!isValid) {
    return;
  }

  const { self, someRef } = validatedRefs;

  return gsap.timeline({
    scrollTrigger: {
        trigger: document.body,
        end: '+=100%',
        scrub: true,
        markers: true,
        pin: self,
    }
  })
  .fromTo(someRef, {
    y: () => isLargeViewport ? 100 : 0,
  })
}

As soon as the isLargeViewport changes the scroll animation is re-created causing the original timeline to get killed (on the useAnimation's useEffect cleanup. This does indeed clean the animations but it causes an extra pin container in the DOM and for some reason the "pinned" element just disappears as soon as the end market hits the end.

Here is the diff that solved my problem:

diff --git a/node_modules/@mediamonks/react-kit/dist/gsap/hooks/useAnimation/useAnimation.js b/node_modules/@mediamonks/react-kit/dist/gsap/hooks/useAnimation/useAnimation.js
index bd5013e..307af3d 100644
--- a/node_modules/@mediamonks/react-kit/dist/gsap/hooks/useAnimation/useAnimation.js
+++ b/node_modules/@mediamonks/react-kit/dist/gsap/hooks/useAnimation/useAnimation.js
@@ -12,6 +12,7 @@ export function useAnimation(callback, dependencies) {
         const _animation = _callback();
         animation.current = _animation;
         return () => {
+            _animation?.scrollTrigger?.kill();
             _animation?.kill();
         };
     }, [_callback]);

This issue body was partially generated by patch-package.

larsvanbraam commented 4 months ago

So @ReneDrie suggested to use the Tween.revert (https://gsap.com/docs/v3/GSAP/Tween/revert()/) method instead as that also kills under the hood.

Its seems that this has the same result but without the extra line of code:

diff --git a/node_modules/@mediamonks/react-kit/dist/gsap/hooks/useAnimation/useAnimation.js b/node_modules/@mediamonks/react-kit/dist/gsap/hooks/useAnimation/useAnimation.js
index bd5013e..607da3e 100644
--- a/node_modules/@mediamonks/react-kit/dist/gsap/hooks/useAnimation/useAnimation.js
+++ b/node_modules/@mediamonks/react-kit/dist/gsap/hooks/useAnimation/useAnimation.js
@@ -12,7 +12,7 @@ export function useAnimation(callback, dependencies) {
         const _animation = _callback();
         animation.current = _animation;
         return () => {
-            _animation?.kill();
+            _animation?.revert();
         };
     }, [_callback]);
     return animation;

Might be worth doing a little bit of extra testing though 🤓

ReneDrie commented 4 months ago

This would be a lovely suggestion, especially when re-initialising transitions as it returns to the default state (if a dependency changes). But I do think it needs some testing indeed. When we cleanup a component, this will revert to the base state. So could this also mean you might see the component 1 frame in the default state again (when animation out especially) before it gets removed from the DOM?