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

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

Merged
merged 1 commit into from
Jan 14, 2024
Merged

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

merged 1 commit into from
Jan 14, 2024

Conversation

lachieh
Copy link
Contributor

@lachieh lachieh commented Jan 4, 2024

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
Copy link
Owner

mifi commented Jan 5, 2024

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
Copy link
Contributor Author

lachieh commented Jan 10, 2024

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
Copy link
Owner

mifi commented Jan 13, 2024

seems that something broke in the tests

@lachieh
Copy link
Contributor Author

lachieh commented Jan 13, 2024

I'll take a look this weekend 👍

@lachieh
Copy link
Contributor Author

lachieh commented Jan 13, 2024

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.

@mifi mifi merged commit aa753c7 into mifi:master Jan 14, 2024
3 checks passed
hongrich pushed a commit to hongrich/react-lottie-player that referenced this pull request Mar 6, 2024
This change was originally made in mifi#111. Pick type only takes two parameters: https://www.typescriptlang.org/docs/handbook/utility-types.html#picktype-keys

The keys should be unioned with `|`. `'path'` and `'animationData'` are separated with `,` which makes them invalid. They also don't need to be picked from `AnimationConfig` since we already add them in explicitly.
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.

ref Type incorrect
2 participants