dkaoster / scrolly-video

Components for scroll-based (or other externally controlled) playback.
https://scrollyvideo.js.org/
MIT License
965 stars 39 forks source link

Request: align `trackScroll` with `videoPercentage` #89

Closed tarsisexistence closed 6 months ago

tarsisexistence commented 7 months ago

So, basically, I imagine it should work with the same percentage value. Let's say we set the percentage to 30% automatically, but then we decided to scroll down with enabled trackScroll. The problem here is that trackScroll relies only on the scroll position rather than videoPercentage, so surprisingly, once you start scrolling, you appear at the beginning of the video with a jump.

dkaoster commented 6 months ago

This seems kind of strange to me. I guess I'm not sure what the use case is here. My thinking is that if trackScroll is enabled, then scrollyVideo should always depend on the scroll position and if someone wanted to build a programmatic change on top of it then it should simply programmatically manipulate the scroll position of the page.

If someone wanted a more complex animation with the video jumping to different parts, they should write their own function that only sets setCurrentTimePercent based on their own scroll listener.

I'm open to being convinced otherwise, but I'd like to see an example of where something like this would be useful.

tarsisexistence commented 6 months ago

@dkaoster, the argument would be pretty straightforward. The current API allows you to have both simultaneously, right? It produced buggy behaviour with the jumps (let's say you used programmatic change first and then started scrolling).

In my opinion, we should provide a good experience for end users, no matter what combination of APIs they choose.

In terms of use case, I use this library for dynamic content based on expectations, i.e. some viewports of the content should auto-scroll the content, and some should be scrolled by a user only (something that Apple does on their websites)

dkaoster commented 6 months ago

In that case, I think I'd prefer to keep it simple by disabling the setCurrentTimePercent if trackScroll is enabled, logging a warning that setCurrentTimePercent has no effect if trackScroll is enabled. I think that the API should not allow you to have both simultaneously.

Thoughts?

tarsisexistence commented 6 months ago

@dkaoster as a simple solution for a library - it can be.

As I shared my context for a business task - I really need both simultaneously. This is an example of a similar thing I want to achieve: https://www.apple.com/ipad-pro/. Mainly, it's scrollable, but at some points, it "auto plays" animation.

So, in my scenario, I also want to have the main trackScroll, but for some viewports, run setCurrentTimePercent. I could deactivate and activate them one by one, but the current implementation makes it impossible because of the different scroll positions.

Overall, I see only positive effects of this change since we don't implement anything new. All we need to do is to change the scroll position via window.scrollTo, and the existing code will do the rest.

Wdyt?

dkaoster commented 6 months ago

@tarsinzer okay. You've convinced me. Let's do this given the fact that there really are no downsides to implementing this.

dkaoster commented 6 months ago

@tarsinzer are we good to close this one?

tarsisexistence commented 6 months ago

@dkaoster absolutely. Thank you for a great contribution experience. It's my pleasure making this library better.