adds ScrollerLottie

[SudevKiyadaTR]

What's in this pull request

adds ScrollerLottie component. Closes #280.

Before submitting, please check that you've ...

• Read our contributing guide
• Documented any new components or features
• Tagged an editor to review this PR

[changeset-bot]

⚠️ No Changeset found

Latest commit: c803927

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[SudevKiyadaTR]

@pkd2512 @MinamiFunakoshiTR While I fix TS issues, what do you guys think of naming this one?

Although it's not a scroller per se and we mostly use it as a Scroller, keeping it simpler can help us avoid complexity for using it as a standalone Lottie.

If we go for making it a scroller-default, it'll add another prop - scrollerHeight and wrap it inside another sticky container.

[MinamiFunakoshiTR]

LottieScroller sounds good to me. We don't use lotties as non-scroller animations often.

I'm reviewing this PR. I'll let you know once I've gone through it.

[MinamiFunakoshiTR]

Hey @SudevKiyadaTR, I'm still reviewing this PR -- I'll make a PR for you to review that will merge into this branch -- but have a few questions.

1. When I resize the page and the lottie container size changes, the animation gets stretched before snapping into a proper aspect ratio. This seems to be coming from the height on canvas being 100%. Is there a way around this?
2. The documentation recommends converting the lottie .json file into .lottie then ultimately to a .zip file. Since we're recommending that people convert the lottie file to a .zip, can you convert the default lottie file (the TR logo animation) into a .zip file? We can keep the other demo files as .lottie files if we're OK with people passing in .lottie files into the component, but if we don't want users to do that, let's avoid using them in all our demos.

[SudevKiyadaTR]

LottieScroller sounds good to me. We don't use lotties as non-scroller animations often.

I'm reviewing this PR. I'll let you know once I've gone through it.

@MinamiFunakoshiTR, do you think we should have a height prop and wrap the existing component inside a div or scrollerBase? Though this might take away the possibility of having a static Lottie animation that doesn't act 'sticky'. If not, am leaning towards ScrollerLottie to follow the other components' convention (ScrollerXYZ) and also a bit easier to make sense on what goes inside ScrollerBase?

On the other questions,

1. I didn't find Lottie-native resize event, but I guess we could listen to the window resize event and maybe pause the render till the user is done resizing?
2. I'll do that.

[MinamiFunakoshiTR]

LottieScroller sounds good to me. We don't use lotties as non-scroller animations often.
I'm reviewing this PR. I'll let you know once I've gone through it.

@MinamiFunakoshiTR, do you think we should have a height prop and wrap the existing component inside a div or scrollerBase? Though this might take away the possibility of having a static Lottie animation that doesn't act 'sticky'. If not, am leaning towards ScrollerLottie to follow the other components' convention (ScrollerXYZ) and also a bit easier to make sense on what goes inside ScrollerBase?

On second thought -- now that I've played around with all the demos more, I think we should just name this <Lottie>, like we do for <Video> and <FeaturePhoto>. This makes more sense to me since the component itself doesn't have a scroller function; we'd need to use it with ScrollerBase etc to add the scroll. What do you think?

On the other questions,

1. I didn't find Lottie-native resize event, but I guess we could listen to the window resize event and maybe pause the render till the user is done resizing?

That makes sense, have a go and see if it fixes it. The native Svelte <svetlte:window onresize> might work.

[SudevKiyadaTR]

Will do that. Should I wait for a PR from your side before I tackle these or safe to make the changes?
@MinamiFunakoshiTR

[MinamiFunakoshiTR]

Go ahead and make those changes and commit them to this PR. I'll deal with any merge conflicts that might pop up in my upcoming PR.

[MinamiFunakoshiTR]

Actually, I ended up renaming ScrollerLottie to Lottie while working on my PR so don't worry about making that change. Go ahead with the other changes, though.

[SudevKiyadaTR]

@MinamiFunakoshiTR much thanks for reviewing the code. Those changes look good!

I made the changes you requested. I don't think there's a workaround to the canvas getting stretched on resize. I have added pause-resume logic but the canvas would anyway get stretched. Another option is to hide-show on resize but not sure if we want that.

Also, looping @hobbes7878, there's a WASM inclusion in the code for perf gains. But I think .wasm isn't yet supported in publisher. Though I see it as an allowed filetype on Connect. Should I get rid of it?

[MinamiFunakoshiTR]

I made the changes you requested. I don't think there's a workaround to the canvas getting stretched on resize. I have added pause-resume logic but the canvas would anyway get stretched. Another option is to hide-show on resize but not sure if we want that.

Thanks. What if we set the lottie container width instead of height, and make height auto?

[SudevKiyadaTR]

What if we set the lottie container width instead of height, and make height auto?

Would be fine for standalone animation but may cause issues when used with ScrollerBase and the animation doesn't match the window aspect ratio. Maybe we keep the prop but init it with auto?

Having a prop would allow users a bit of flexibility in terms of what size they want to render. I mean would give that flexibility more priority over window resize.

[MinamiFunakoshiTR]

What if we set the lottie container width instead of height, and make height auto?

Would be fine for standalone animation but may cause issues when used with ScrollerBase and the animation doesn't match the window aspect ratio. Maybe we keep the prop but init it with auto?

Having a prop would allow users a bit of flexibility in terms of what size they want to render. I mean would give that flexibility more priority over window resize.

Got it. can you play around with the height and width props? I agree it would be nice to add flexibility. I would default to making width fluid and height auto -- my hunch is that that's a more common use case. But it would be great if you could test it out in demos.

[MinamiFunakoshiTR]

Hi! Just wanted to check in on where we're at with this. Do you need anything from me?

[SudevKiyadaTR]

Hi! Just wanted to check in on where we're at with this. Do you need anything from me?

Nope, we good. I just made some changes and removed wasm support. Users can still opt-in for wasm support by using player.setWasmUrl(url)

[MinamiFunakoshiTR]

if everything looks good after a final check, go ahead and merge this with a changeset. thank you for all your hard work!