fix: react wrapper - Githubissues

dkaoster / scrolly-video

Components for scroll-based (or other externally controlled) playback.

https://scrollyvideo.js.org/

MIT License

971 stars 39 forks source link

fix: react wrapper #84

Closed tarsisexistence closed 9 months ago

tarsisexistence commented 9 months ago

Based on #83.

I decided to fix it by myself to make unblock myself quicker. It turns out the implementation didn't consider react lifecycle specifics, and the library itself wasn't happy about it producing breaking behavior.

In addition, I decided to improve the react demo.

Also, I would like to add prettier for easier code style management, but I wonder whether you are good with it.

If you need a detailed breakdown and explanation of the changed LoCs - just let me know.

tarsisexistence commented 9 months ago

By the way, I would like to mention two more things:

I think demos should read the local version of the library, not the npm. Then it would be easier to check whether it works fine or not.
I see you bumped 0.0.17 on 09.08.23, but it looks like you still need to publish it.

dkaoster commented 9 months ago

Thank you for this! I am pretty busy the next couple of days but will review as soon as possible!

dkaoster commented 9 months ago

Hi @tarsinzer!

Thank you so much for this, I think the changes to the react component are excellent.

Regarding the demos, I built the demos as the minimum barebones shell that someone could copy and paste directly into their own project to get started, which is why I have the demos set to read from npm and not locally. If someone were to simply copy paste the react code onto their computer and run it, having the package linked locally would break the starter code. And in the same vein, I purposely left the demos blank without controls or any of those other bells and whistles.

That said, maybe there's a separate folder I can add for developing and testing the various framework components.

tarsisexistence commented 9 months ago

I was also curious if I could add prettier to the project. Then, it would be easier for contributors to align with code style and auto formatters, especially after Eslint decides to move away from formatting the files and focus on linting only.

One more thing I want to ask regarding the API: is there any chance I can manipulate it with speed linearly, like default video play? Let's say I want to play video naturally from 0% to 15%, and then enable the default jump to the end after some delay (with the default transitionSpeed of 8). I was just playing with numbers and couldn't achieve linear easing behaviour. If I set the transition speed to 1, it plays normally but ends very slowly... not something what I'm looking for.

dkaoster commented 9 months ago

@tarsinzer sure! If you'd like to add prettier go ahead. I'm happy to merge that into the codebase.

Regarding easing, maybe there's a way we can add an option that takes one of D3's easing functions that it uses for easing. That way, anyone can specify their own custom easing function to use.

Feel free to open more PRs, I'm happy to review!