search

mifi / react-lottie-player

Fully declarative React Lottie player
MIT License
494 stars 52 forks source link

expose div ref and update types for Lottie.ref #111

Closed lachieh closed 7 months ago

lachieh commented 7 months ago

Closes #110

I needed to update the lottie-web version to get the correct types, but I don't use yarn so I installed latest and it looks like it updated the lock file quite significantly. Let me know if this is not good and I'll leave the changes to yarn.lock off.

mifi commented 7 months ago

Thanks for the PR. Yes I tihnk you can remove all the yarn-related changed from the pr, as well as the lottie-web version change is package.json because it doesn't do anything. I might have misunderstood but I thought the change was a TS-only fix. Why do we need to expose the animElementRef? Nobody requested the need for the div element to be exposed as a ref (until now). I'm not sure if we'll want that to be part of the API. What's the use case for it?

lachieh commented 7 months ago

Thanks, ditched the yarn changes and element ref changes. I left the function handling in with the existing ref, but can take that out too if preferred.

mifi commented 7 months ago

seems that something broke in the tests

lachieh commented 7 months ago

I'll take a look this weekend 👍

lachieh commented 7 months ago

Ok, sorted. Tests run successfully after failing locally so should be good for another run.

I've added some basic type information to the makeLottiePlayer.js file, but I can remove if you'd prefer.