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

Setup Lottie #14393

Merged
merged 13 commits into from
Feb 23, 2023
Merged

Setup Lottie #14393

merged 13 commits into from
Feb 23, 2023

Conversation

roryabraham
Copy link
Contributor

@roryabraham roryabraham commented Jan 18, 2023

Details

Sets up Lottie animations in E/App. Replaces the two gifs we already have in place with their Lottie equivalent.

I wanted to set up .lottie files because they're only about 10% the size of their .json equivalent, but it looks like it's going to be a bit more involved because neither lottie-react-native nor react-native-web-lottie currently support that format. Will address this in a follow-up issue, but I don't think it's critical right now.

Fixed Issues

$ (partial) #12261

Tests

  1. Hardcode this line to true
  2. Open the app, then create a workspace if needed.
  3. Go to workspace settings -> reimburse expenses -> connect bank account
  4. Verify that you see the scanning bank info animation and it looks good.
  5. Change this line to replace ROUTES.SETTINGS_PAYMENTS with ROUTES.ENABLE_PAYMENTS.
  6. Comment out these lines and replace these lines with just: <ActivateStep userWallet={this.props.userWallet} />
  7. Then open the settings menu and click on Payments. Verify that you see the scanning animation and it looks good.
  8. Replace isGoldWallet on this line with true.
  9. Verify that you now see the fireworks animation and it looks good.
  • Verify that no errors appear in the JS console

Offline tests

None.

QA Steps

This should be covered by regression tests. As you go through the regression tests for wallet setup and bank account connection, verify that the animations appear as they normally do.

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
EnableScanning.mov
EnableFireworks.mov
WorkspaceLoading.mov
Mobile Web - Chrome
screen-20230210-131750.mp4
screen-20230210-131723.mp4
screen-20230210-131647.mp4
Mobile Web - Safari
RPReplay_Final1676063478.MP4
RPReplay_Final1676063454.MP4
RPReplay_Final1676063396.MP4
Desktop
EnableFireworks.mov
EnableScanning.mov
WorkspaceScanning.mov
iOS
RPReplay_Final1676060397.MP4
RPReplay_Final1676061300.MP4
RPReplay_Final1676061390.MP4
Android
screen-20230210-130106.mp4
screen-20230210-130142.mp4
screen-20230210-130220.mp4

@MelvinBot
Copy link

Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work?

@roryabraham roryabraham changed the title [WIP] Setup Lottie Setup Lottie Feb 10, 2023
@roryabraham roryabraham marked this pull request as ready for review February 10, 2023 21:23
@roryabraham roryabraham requested a review from a team as a code owner February 10, 2023 21:23
@melvin-bot melvin-bot bot requested review from stitesExpensify and removed request for a team February 10, 2023 21:23
@MelvinBot
Copy link

@stitesExpensify Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@OSBotify
Copy link
Contributor

🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪

android 🤖 iOS 🍎
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/14393/index.html https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/14393/index.html
Android iOS
desktop 💻 web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/$PULL_REQUEST_NUMBER/NewExpensify.dmg https://$PULL_REQUEST_NUMBER.pr-testing.expensify.com
Desktop Web

@mountiny mountiny self-requested a review February 16, 2023 15:03
@roryabraham
Copy link
Contributor Author

Bump – don't want this getting too stale

@mountiny
Copy link
Contributor

mountiny commented Feb 23, 2023

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
lottieWeb.mp4
Mobile Web - Chrome
lottieAndroidWeb.mp4
Mobile Web - Safari
lottieIOSweb.mp4
Desktop
lottieDesktop.mp4
iOS

see below

Android

running into some unrelated issues with the emulator, in the PR body it seems to be working great so I wont hold on this

@mountiny
Copy link
Contributor

Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great, thanks for working on this! To push this forward, I will merge the PR since I have tested it on all platforms and it seems to work great. Only tested one page but QA can catch the other ones

@mountiny mountiny merged commit 08f89f3 into main Feb 23, 2023
@mountiny mountiny deleted the Rory-Lottie branch February 23, 2023 16:36
@github-actions
Copy link
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 703.520 ms → 709.013 ms (+5.493 ms, +0.8%)
App start regularAppStart 0.013 ms → 0.014 ms (+0.001 ms, +8.2%)
App start nativeLaunch 20.594 ms → 20.581 ms (-0.013 ms, ±0.0%)
App start runJsBundle 192.484 ms → 190.933 ms (-1.551 ms, -0.8%)
Open Search Page TTI 618.938 ms → 609.342 ms (-9.596 ms, -1.6%)
Show details
Name Duration
App start TTI Baseline
Mean: 703.520 ms
Stdev: 30.231 ms (4.3%)
Runs: 653.2484509982169 660.2190370000899 662.5205769985914 666.0986380018294 670.014214001596 673.1497309990227 676.0956889986992 683.1046429984272 683.6059119999409 687.6673020012677 688.6570140011609 689.8906349986792 694.6555260010064 696.3612800016999 697.1090440005064 700.6955090016127 704.0043330006301 704.7687989994884 711.38108599931 712.5624760016799 714.6031740009785 716.1580590009689 716.9128810018301 719.029036000371 720.4232879988849 726.3737539984286 732.8454810008407 746.7417440004647 762.8310809992254 768.4462779983878 768.9607819989324

Current
Mean: 709.013 ms
Stdev: 24.351 ms (3.4%)
Runs: 666.6611480005085 670.2105179987848 675.5430510006845 680.6531610004604 682.2993529997766 682.5217129997909 687.5675790011883 688.865242999047 692.2193499989808 703.1636769995093 703.8156530000269 704.6316669993103 705.055610999465 706.3966200016439 709.9506400004029 711.1313650012016 713.8447909988463 716.5793480016291 717.5781229995191 720.5793099999428 721.8470989987254 724.0426289997995 726.3022349998355 727.2427270002663 731.1782299987972 733.1452630013227 737.1813910007477 740.5331399999559 780.6368310004473
App start regularAppStart Baseline
Mean: 0.013 ms
Stdev: 0.001 ms (4.6%)
Runs: 0.011799998581409454 0.01212499663233757 0.012206997722387314 0.012207001447677612 0.012248001992702484 0.01228800043463707 0.01228800043463707 0.012777000665664673 0.012818001210689545 0.012818001210689545 0.012938998639583588 0.012939002364873886 0.012940000742673874 0.013020999729633331 0.013102002441883087 0.013225000351667404 0.013305999338626862 0.013345997780561447 0.013346001505851746 0.013386998325586319 0.01342799887061119 0.013468001037836075 0.013468999415636063 0.013509001582860947 0.013509001582860947 0.013550002127885818 0.013631001114845276 0.01371300220489502 0.013794001191854477 0.014038000255823135 0.014038000255823135 0.014119002968072891

Current
Mean: 0.014 ms
Stdev: 0.001 ms (4.4%)
Runs: 0.01297999918460846 0.013020999729633331 0.013225000351667404 0.013509999960660934 0.013590000569820404 0.013590998947620392 0.013631001114845276 0.01367199793457985 0.013794001191854477 0.013794001191854477 0.013794001191854477 0.013875000178813934 0.014038000255823135 0.014077998697757721 0.01411999762058258 0.01411999762058258 0.014159999787807465 0.014201000332832336 0.01448499783873558 0.014485001564025879 0.014607001096010208 0.014607999473810196 0.014607999473810196 0.014607999473810196 0.014608003199100494 0.014729999005794525 0.014811001718044281 0.015014000236988068 0.015095997601747513 0.015258997678756714 0.01534000039100647
App start nativeLaunch Baseline
Mean: 20.594 ms
Stdev: 2.914 ms (14.1%)
Runs: 17 18 18 18 18 18 18 18 19 19 19 19 19 19 20 20 20 20 20 20 20 20 21 21 23 23 23 23 25 27 28 28

Current
Mean: 20.581 ms
Stdev: 3.067 ms (14.9%)
Runs: 18 18 18 18 18 18 18 18 18 18 19 19 19 20 20 20 20 20 20 20 20 20 21 21 22 23 25 25 27 28 29
App start runJsBundle Baseline
Mean: 192.484 ms
Stdev: 19.117 ms (9.9%)
Runs: 160 163 165 170 172 178 179 180 183 184 184 185 185 185 188 189 190 192 192 193 198 198 200 202 209 212 220 223 225 225 238

Current
Mean: 190.933 ms
Stdev: 13.249 ms (6.9%)
Runs: 173 174 174 176 177 181 181 181 181 183 185 186 186 187 188 188 189 190 192 193 196 196 200 201 201 203 206 214 217 229
Open Search Page TTI Baseline
Mean: 618.938 ms
Stdev: 32.725 ms (5.3%)
Runs: 572.7086180001497 579.1038010008633 580.8068850003183 587.8239750005305 589.9721270017326 590.6926679983735 592.0700680017471 594.0335690006614 595.5668129995465 596.821654997766 598.6212970018387 599.4956469982862 599.780965000391 601.9456389993429 602.3862709999084 608.6273600012064 611.4626870006323 613.921061001718 615.9430750012398 616.3625489994884 616.3868819996715 616.8804120011628 625.8549800030887 629.0808920003474 640.2757980003953 647.6081140004098 649.2826339974999 649.9260670021176 658.178752001375 672.6239020004869 676.9503169991076 696.1426189988852 697.6115309968591

Current
Mean: 609.342 ms
Stdev: 15.680 ms (2.6%)
Runs: 572.0400389991701 580.6116949990392 585.2578939981759 587.3935139998794 594.665160998702 596.9667970016599 599.6183679997921 600.7425950020552 601.8256839998066 602.671996999532 606.8214929997921 606.9163010008633 607.5277509987354 607.6428230032325 607.7015389986336 608.3103439994156 608.3215739987791 608.7200520001352 609.3011480011046 611.2707519978285 611.5411380007863 620.6671150028706 621.4179279990494 622.1898190006614 622.5851239971817 622.7110190019011 626.9084879979491 630.7627769969404 631.7462159991264 632.9992679990828 641.7490650005639

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/mountiny in version: 1.2.76-5 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.2.76-7 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 failure ❌
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

1 similar comment
@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.2.76-7 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 failure ❌
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.2.76-7 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@abdulrahuman5196
Copy link
Contributor

There is a console error bug while we defined the animation: PropTypes.string but we where passing type of object to it. This has been found and rectified as part of this issue - #19365

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants