mifi / react-lottie-player

Fully declarative React Lottie player
MIT License
505 stars 53 forks source link

add support for lottie light player #39

Closed simonpkerr closed 3 years ago

simonpkerr commented 3 years ago

We are using this great package in our component library but rollup doesn't like eval, which the full lottie player uses. This pr reworks the code a little to export a version of the player that uses lottie light, which doesn't use eval, and is much smaller than the full player, which makes rollup happy. I've added tests for the light player, which seem to pass ok. Obviously the light player isn't going to support every option that the full player supports, but I guess that's up to the consumer. Existing functionality should remain unchanged.

mifi commented 3 years ago

Thanks for this! I never heard about lottie light. It seems like an undocumented API? I found something here https://github.com/airbnb/lottie-web/issues/289 Still I'm open to merging this, because eval is indeed a bit scary. Is it possible to change the PR so it doesn't reformat all the code? that way maybe it will be easier to see which lines of code have changed.

simonpkerr commented 3 years ago

yeah, the docs are very sketchy around it, but this seems to be the only way to avoid having eval. apps that have a strict CSP would be severely limited in using lottie due to this. i'll take a look at the formatting. i think its just indented the main function since I wrapped it with that factory method. the code for the component is unchanged. i'll take a look though

simonpkerr commented 3 years ago

yeah, probably not much I can do about the indentation unless you can see something I've missed. The main component is just wrapped in a factory function that allows the various entrypoints to choose which version of the base lottie player will be used. if I moved the component out of this function, it won't have access to the right lottie player.

mifi commented 3 years ago

If all you did was to wrap the code in the function, then you (or I) can just revert, then redo that change without applying the formatting changes (just only change indentations). I'll have a look at it when i have some time.

simonpkerr commented 3 years ago

Oh I see what you mean. Yeah, I can update that.

On 23 Jul 2021, at 09:42, Mikael Finstad @.***> wrote:

 If all you did was to wrap the code in the function, then you (or I) can just revert, then redo that change without applying the formatting changes (just only change indentations). I'll have a look at it when i have some time.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

simonpkerr commented 3 years ago

If all you did was to wrap the code in the function, then you (or I) can just revert, then redo that change without applying the formatting changes (just only change indentations). I'll have a look at it when i have some time.

Ok, so I've reverted the formatting changes. Unfortunately not much has changed since the main lottie player is all indented. can you see anything else I could do to sort that?

simonpkerr commented 3 years ago

Hi @mifi , is this ok now or do you need anything else changing? thanks

mifi commented 3 years ago

Thanks. Looks good now. I will do some minor improvements then merge. Btw I noticed that the lottie light rendered has more jagged edges than the standard one. Do you know why that is?

lottie-player-test-js-lottie-player-screenshots-lottie-player-light-renders-with-animation-data-1-snap lottie-player-test-js-lottie-player-screenshots-lottie-player-main-renders-with-animation-data-1-snap