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.

Sign up for GitHub

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

feat(setScrollPercent): align trackScroll with videoPercentage #91

Merged
merged 4 commits into from
Mar 28, 2024
Merged

feat(setScrollPercent): align trackScroll with videoPercentage #91

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3b53e92
feat: add manual scroll feature to synchronise progress of percentage…
tarsisexistence Mar 3, 2024
8a0ac18
feat: update implementations
tarsisexistence Mar 3, 2024
4818c3d
Merge branch 'forkmain' into scroll
tarsisexistence Mar 24, 2024
1e78d9a
refactor: prevent invoking setScrollPercent when trackScroll=false
tarsisexistence Mar 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/ScrollyVideo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,30 @@ class ScrollyVideo {
this.transitionToTargetTime(options);
}

/**
* Simulate trackScroll programmatically (scrolls on page by percentage of video)
*
* @param percentage
*/
setScrollPercent(percentage) {
Copy link
Owner

Choose a reason for hiding this comment

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

Two comments:

  1. Can we have this function also check if trackScroll is true before running? This function is technically exposed to any ScrollyVideo instance, and although it is never meant to be used directly, someone may discover it and weird things would happen if you tried calling it while trackScroll is false.
  2. Also, it seems like the way this is currently implemented, syncing videoPercentage with the scroll position would not work for the vanilla js implementation, so we may need to rethink how this is implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dkaoster updated PR about the first point.
Can you explain a little bit more in detail about the second one?

Copy link
Owner

Choose a reason for hiding this comment

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

Because in the react, vue, and svelte versions you've added the following logic in the components and outside of the library itself:

      if (trackScroll) {
        scrollyVideoRef.current.setScrollPercent(videoPercentage)
      } else {
        scrollyVideoRef.current.setTargetTimePercent(videoPercentage);
      }

So if someone were to implement ScrollyVideo without using one of these frontend libraries, they would have to recreate this logic themselves every time. I'm wondering if there is a way to integrate this into ScrollyVideo.js itself instead of duplicating it across react, vue, and svelte separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dkaoster I think this is the same concern to the change of videoPercentage that automatically invokes setTargetTimePercent, isn't it? 🤔 I mean it looks identical 🤔

Addressing your concern, in React examples you manipulate with a progress that automatically runs this condition on change. In JS code, you don't have automatic effects on state change, so you would handle it manually via scrollyVideo.setScrollPercent

Btw, I want to do another PR of onProgress callback that allows users from outside understand on which stage are they now. It's possible to do if you don't have trackScroll, so you control the progress manually, but as you see, I connect trackScroll with programmatic handling.

If i'm missing something, please let me know.
Would be happy to make things better :)

Copy link
Owner

Choose a reason for hiding this comment

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

sorry, you're right. Ignore my last comment. I think this looks good to me!

if (!this.trackScroll) {
console.warn('`setScrollPercent` requires enabled `trackScroll`');
return;
}

const parent = this.container.parentNode;
const { top, height } = parent.getBoundingClientRect();

// eslint-disable-next-line no-undef
const startPoint = top + window.pageYOffset;
// eslint-disable-next-line no-undef
const containerHeightInViewport = height - window.innerHeight;
const targetPoint = startPoint + containerHeightInViewport * percentage;

// eslint-disable-next-line no-undef
window.scrollTo({ top: targetPoint });
}

/**
* Call to destroy this ScrollyVideo object
*/
Expand Down
15 changes: 11 additions & 4 deletions src/ScrollyVideo.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import React, {
forwardRef,
useEffect,
useImperativeHandle,
useRef,
useState,
useRef,
useImperativeHandle,
} from 'react';
import ScrollyVideo from './ScrollyVideo';

Expand Down Expand Up @@ -75,9 +75,13 @@ const ScrollyVideoComponent = forwardRef(function ScrollyVideoComponent(
videoPercentage >= 0 &&
videoPercentage <= 1
) {
scrollyVideoRef.current.setTargetTimePercent(videoPercentage);
if (trackScroll) {
scrollyVideoRef.current.setScrollPercent(videoPercentage)
} else {
scrollyVideoRef.current.setTargetTimePercent(videoPercentage);
}
}
}, [videoPercentage]);
}, [videoPercentage, trackScroll]);

// effect for unmount
useEffect(
Expand All @@ -95,6 +99,9 @@ const ScrollyVideoComponent = forwardRef(function ScrollyVideoComponent(
setTargetTimePercent: scrollyVideoRef.current
? scrollyVideoRef.current.setTargetTimePercent.bind(instance)
: () => {},
setScrollPercent: scrollyVideoRef.current
? scrollyVideoRef.current.setScrollPercent.bind(instance)
: () => {},
}),
[instance],
);
Expand Down
13 changes: 11 additions & 2 deletions src/ScrollyVideo.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,17 @@
}

// If we need to update the target time percent
if (scrollyVideo && typeof videoPercentage === 'number' && videoPercentage >= 0 && videoPercentage <= 1) {
scrollyVideo.setTargetTimePercent(videoPercentage);
if (
scrollyVideo &&
typeof videoPercentage === 'number' &&
videoPercentage >= 0 &&
videoPercentage <= 1
) {
if (restProps.trackScroll) {
scrollyVideo.setScrollPercent(videoPercentage);
} else {
scrollyVideo.setTargetTimePercent(videoPercentage);
}
}
}
}
Expand Down
6 changes: 5 additions & 1 deletion src/ScrollyVideo.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,11 @@ export default {
videoPercentage >= 0 &&
videoPercentage <= 1
) {
this.scrollyVideo.setTargetTimePercent(videoPercentage);
if (restProps.trackScroll) {
this.scrollyVideo.setScrollPercent(videoPercentage);
} else {
this.scrollyVideo.setTargetTimePercent(videoPercentage);
}
}
},
deep: true,
Expand Down