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

[HOLD for payment 2023-11-21] Implement web support in lottie-react-native #26871

Closed
roryabraham opened this issue Sep 6, 2023 · 24 comments
Closed
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production NewFeature Something to build that is a new item. Weekly KSv2

Comments

@roryabraham
Copy link
Contributor

Problem

We recently sponsored support for .lottie files in lottie-react-native. However, we currently use react-native-web-lottie to provide a unified interface for lottie across native and web.

However, react-native-web-lottie is unmaintained, and the patch we have for it is basically a complete rebuild of this simple lib.
Now we'd like to start using .lottie files, and there is an existing dotlottie player for web, but at this point I don't think it makes sense to continue with react-native-web-lottie.

Solution

Let's build web support into lottie-react-native for both lottie JSON and .lottie file formats.

@roryabraham roryabraham added Weekly KSv2 NewFeature Something to build that is a new item. labels Sep 6, 2023
@roryabraham roryabraham self-assigned this Sep 6, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 6, 2023

@roryabraham
Copy link
Contributor Author

cc @wojtus7

@wojtus7
Copy link
Contributor

wojtus7 commented Sep 6, 2023

Hi!

I would like to take care of this task 🥰

Overall my idea is to:

  • Create web examples in lottie-react-native repo
  • Add web implementations to make web versions of the example app work the same as android/iOS/windows equivalents

Then create PR for lottie-react-native and after it is hopefully merged use it in Expensify (also removing react-native-web-lottie library)

@kosmydel
Copy link
Contributor

Hey, @roryabraham as I am working with @wojtus7 on this task I have some questions about this issue:

  1. I want to make sure, that we are on the same page: We want to add the dotlottie library to the lottie-react-native library (as a dependency), and then use it to support the web version.
  2. Do we need plain .json support? The dotlottie library supports only .lottie files. I can use lottie-react library, to support JSON files as well.

@roryabraham
Copy link
Contributor Author

I want to make sure, that we are on the same page: We want to add the dotlottie library to the lottie-react-native library (as a dependency), and then use it to support the web version.

Yes

Do we need plain .json support? The dotlottie library supports only .lottie files. I can use lottie-react library, to support JSON files as well.

Yes, we need plain .json support as well to upstream our efforts.


Thanks!

@roryabraham
Copy link
Contributor Author

@wojtus7 @kosmydel what's the latest here?

@kosmydel
Copy link
Contributor

@wojtus7 @kosmydel what's the latest here?

Hey, I'm currently busy with other tasks. But here is an update:

We have some working draft prepared here. We still have to do some cleanup, refactoring, and make all props (that are possible) to work on the web.

@kosmydel
Copy link
Contributor

Hey @roryabraham, short update:
I've prepared a draft of the PR for the upstream.
I think that we should first upgrade the lottie-react-native library in the App to the latest version (6.3.1) because it has some breaking changes. We have to do that anyway, so the sooner is better. In that way, I will be able to focus only on the web implementation, instead of upgrading the library.

FYI I'm going to be OOO next week, so @wojtus7 will handle this issue during my absence.

@roryabraham
Copy link
Contributor Author

Still in development. @kosmydel was OOO

@melvin-bot melvin-bot bot removed the Overdue label Oct 2, 2023
@wojtus7
Copy link
Contributor

wojtus7 commented Oct 4, 2023

@roryabraham Since I'm going OOO for two weeks and @kosmydel already kinda took lead in this task I think this is reasonable to change assigned person ☺️

@kosmydel
Copy link
Contributor

kosmydel commented Oct 4, 2023

Hey @roryabraham,
I'm sorry for the delay. I was catching up with my other tasks.

During our internal SWM review, I got a suggestion, that I can use expo to create a webpack.config.js file for the web instead of writing a custom one. It would be shorter and cleaner like here, and it might support hot library reloading in the demo. If we have time, I can have a look at it tomorrow and see how much time it would take. Otherwise, if it is urgent, we can continue with our current approach.

@roryabraham
Copy link
Contributor Author

@kosmydel I don't to see how that's relevant to this task at all. This ticket is focused on building a web implementation in lottie-react-native.

@melvin-bot melvin-bot bot added the Overdue label Oct 20, 2023
@roryabraham
Copy link
Contributor Author

Upstream PR was merged three days ago

@melvin-bot melvin-bot bot removed the Overdue label Oct 20, 2023
@melvin-bot melvin-bot bot added the Overdue label Oct 30, 2023
@roryabraham
Copy link
Contributor Author

6.4.0 was released with web support yesterday, thanks @kosmydel and @matinzd!

@melvin-bot melvin-bot bot removed the Overdue label Oct 30, 2023
@ishpaul777
Copy link
Contributor

ishpaul777 commented Oct 31, 2023

@roryabraham Coming from #27611, this issue requires a patch to react-native-web-lottie specifically to be able to use methods like play() etc. but looks like we are real close to depriciating the package, Can you update when can we expect lottie-react-native package upgrade?

@kosmydel
Copy link
Contributor

kosmydel commented Nov 2, 2023

Hey @ishpaul777, a PR with the lottie-react-native upgrade to 6.4.0 is prepared and waiting for a review. I think it is possible that it will be merged by the end of next week.

@ishpaul777
Copy link
Contributor

Thank you for the update @kosmydel

@roryabraham
Copy link
Contributor Author

#30772 was merged, so once that goes to production this issue can be considered complete

@ishpaul777
Copy link
Contributor

ishpaul777 commented Nov 2, 2023

The animation container changes size after user is navigatied to the page tested on mweb safari

Screen.Recording.2023-11-03.at.1.08.49.AM.mov

Root cause:
Aspect ration style is only applied to the LottieView when we set the auto play to be true, means (animation is playing) so animation changes it aspect ratio when it starts playing. when animation is still (not playing) the style is not applied.
setted autoplay to false and min-height to animation container(unrelated)

Screenshot 2023-11-03 at 1 37 26 AM

cc @kosmydel

@kosmydel
Copy link
Contributor

kosmydel commented Nov 6, 2023

@ishpaul777 thanks for noticing that! A PR with a fix.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Nov 6, 2023
@ishpaul777
Copy link
Contributor

ishpaul777 commented Nov 6, 2023

@kosmydel another issue applying changes from #30901, just incase you haven't noticed yet there's too much gap between animation and Hero Text

Screenshot 2023-11-07 at 1 37 23 AM

@roryabraham roryabraham removed the Reviewing Has a PR in review label Nov 7, 2023
Copy link

melvin-bot bot commented Nov 10, 2023

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@melvin-bot melvin-bot bot added the Overdue label Nov 14, 2023
@melvin-bot melvin-bot bot removed the Overdue label Nov 14, 2023
@github-project-automation github-project-automation bot moved this from HIGH to Done in [#whatsnext] #quality Nov 14, 2023
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Nov 14, 2023
@melvin-bot melvin-bot bot changed the title Implement web support in lottie-react-native [HOLD for payment 2023-11-21] Implement web support in lottie-react-native Nov 14, 2023
Copy link

melvin-bot bot commented Nov 14, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.98-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-11-21. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

  • @kosmydel does not require payment (Contractor)

Copy link

melvin-bot bot commented Nov 14, 2023

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@kosmydel] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
  • [] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production NewFeature Something to build that is a new item. Weekly KSv2
Projects
Development

No branches or pull requests

6 participants