-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat: lottie animations prefetching #32789
feat: lottie animations prefetching #32789
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using FontPreloadPlugin
to prefetch lottie animations seems a little hacky to me 😅 Isn't there a library that is more general and can cache fonts and animations without making a patch?
That being said I think code looks solid, and I don't want to block this PR, so LGTM 👍
I'm setting it as a draft, as we've changed the approach. We want to use a more general library, instead of patching the current one dedicated to fonts. |
Hey, I've switched to the new library ( Probably due to that, the snyk tests are failing. cc @roryabraham |
@kosmydel Doesn't look like it – the snyk failures seem to be the same |
So let's work on getting this ready for review and C+ approved |
Hey, sorry for the delay. The main had problems with the Android, and I couldn't test it after merging. Now it is ready for a review. |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb Safari |
Hey @roryabraham, does this PR need a C+ review? |
@mollfpr, was just going to say I don't think we need a C+ review here, thanks |
Looks good, tests well. I would love to see us do a follow-up to get caching headers working with the webpack dev server |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.4.17-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.17-8 🚀
|
Details
This PR adds Lottie animations prefetching on the Web, mWeb and Desktop platforms. The first time we see the animation, the app should use a prefetched file. Next loads should use cache control headers, but those are set on Cloudflare, so they don't work on the dev environment.
Result:
Fixed Issues
$ #31196
PROPOSAL: N/A
Tests
Note:
Offline tests
This affects only Web, mWeb, and Desktop platforms. I wasn't able to test mWeb offline on iOS as the simulator doesn't support offline mode.
Networking
tab of the browser, if the.lottie
files were loaded.Important note: In the dev environment the prefetching works only for the first loading of the animation.
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
mweb-android.mov
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop2.mov
desktop.mov