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

feat(setScrollPercent): align trackScroll with videoPercentage #91

Merged
merged 4 commits into from
Mar 28, 2024

Conversation

tarsisexistence
Copy link
Contributor

@tarsisexistence tarsisexistence commented Mar 3, 2024

This PR addresses this issue #89.

By design, I add a new callback that replaces setTargetTimePercent because it manipulates with currentTime which doesn't have a sync with a scroll position (described in issue #89).

So, here we

  • keep scroll listener of handler updateScrollPercentage that uses setTargetTimePercent under the hood
  • keep setTargetTimePercent for non-trackScroll implementation (even though now it's possible to enable both at the same time)
  • introduce setPercentage in conjunction with trackScroll, so here we unlock the possibility to use track scroll and programmatic scroll in the same flow.

@tarsisexistence tarsisexistence marked this pull request as draft March 3, 2024 12:34
@tarsisexistence tarsisexistence changed the title [WIP] feat: align trackScroll with videoPercentage Feat: align trackScroll with videoPercentage Mar 3, 2024
@tarsisexistence
Copy link
Contributor Author

@dkaoster, I wonder whether we should adjust anything else here?

And I should probably add the support for callbacks of other implementations (like Svelte)

@tarsisexistence
Copy link
Contributor Author

@dkaoster I decided to support videoPercentage on instance as well. So if videoPercentage is changed with enabled trackScroll - we prefer setPercentage instead of setTargetTimePercent

I did the same for Svelte and Vue. I need help to validate them. I would appreciate your review.

*
* @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!

@tarsisexistence tarsisexistence marked this pull request as ready for review March 24, 2024 15:23
@dkaoster dkaoster merged commit 58e4a0d into dkaoster:main Mar 28, 2024
@tarsisexistence tarsisexistence changed the title Feat: align trackScroll with videoPercentage Feat(setScrollPercent): align trackScroll with videoPercentage Mar 31, 2024
@tarsisexistence tarsisexistence changed the title Feat(setScrollPercent): align trackScroll with videoPercentage feat(setScrollPercent): align trackScroll with videoPercentage Mar 31, 2024
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.

2 participants