Skip to content

Add VBA loading animation#4465

Merged
mountiny merged 7 commits into
mainfrom
vit-addLoadingAnimation
Aug 10, 2021
Merged

Add VBA loading animation#4465
mountiny merged 7 commits into
mainfrom
vit-addLoadingAnimation

Conversation

@mountiny

@mountiny mountiny commented Aug 6, 2021

Copy link
Copy Markdown
Contributor

cc @MitchExpensify

Details

Adds an animation in between VBA steps to make the UI more intuitive.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/172797

Tests

  1. Create a new workspace.
  2. Navigate to it and click Get Started
  3. Choose Connect Manually
  4. Fill in the Routing number and Account number and click Save & Continue
  5. Fill in all the company information and click Save & Continue
  6. Make sure the loading page/animation looks like on the videos

Additionally, can you please test on physical Android device, if the gif animation really works between the steps of VBA flow? In emulator, it has been static image, but the entire app is very slow in my emulator so not sure what has caused it.

QA Steps

Same as tests.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android - only emulator

Screenshots

Web

web.mov

Mobile Web

mobile_web.mov

Desktop

desktop.mov

iOS

iphone.mov

Android

android.mov

@mountiny mountiny requested a review from a team as a code owner August 6, 2021 19:22
@mountiny mountiny self-assigned this Aug 6, 2021
@MelvinBot MelvinBot requested review from TomatoToaster and removed request for a team August 6, 2021 19:22
@mountiny

mountiny commented Aug 6, 2021

Copy link
Copy Markdown
Contributor Author

I will need to follow these steps to make sure the gif works well on Android too.

i will try to update this over weekend. I will be ooo next Monday.

@mountiny mountiny changed the title [WIP] Add translations for VBA loading animation [WIP] Add VBA loading animation Aug 10, 2021
@mountiny mountiny changed the title [WIP] Add VBA loading animation Add VBA loading animation Aug 10, 2021
@mountiny

Copy link
Copy Markdown
Contributor Author

@TomatoToaster This should be ready for review!

@mountiny

Copy link
Copy Markdown
Contributor Author

I have not been able to test it on Android, my setup has gone wild and can't run it, but that is not related to the changes here.

@mountiny

Copy link
Copy Markdown
Contributor Author

@TomatoToaster Alright, I have managed to make my android emulator run it by uninstalling the app from the emulator and then running it again.

It is extremely sluggish which is as always I guess but then the gif of the animation does not move at all. So I do not know if that is just caused by the sluggishness of the system or the gif cant be animated on Android. Are you able to test it on physical device? I do not have any :/

Thank you for the review!

@TomatoToaster TomatoToaster left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left some comments but they seem NAB. Except the generated file one asked about it in slack.

Comment thread src/components/VBALoadingIndicator.js Outdated
Comment thread src/pages/ReimbursementAccount/ReimbursementAccountPage.js Outdated
Comment thread android/app/src/main/java/com/expensify/chat/generated/BasePackageList.java Outdated
@TomatoToaster

Copy link
Copy Markdown
Contributor

It is extremely sluggish which is as always I guess but then the gif of the animation does not move at all. So I do not know if that is just caused by the sluggishness of the system or the gif cant be animated on Android. Are you able to test it on physical device? I do not have any :/

I don't have a physical device to test on. Hmm instead of blocking on that though, can you add in the Tests/QA section to explicitly test this on and android device and to make note if it's frozen. Even if the gif freezes up just for android, I don't think it should block this from being merged. We can explore in a follow-up/regression how to fix that particular issue. Hopefully, it's just a quirk of the emulator.

Do you think that makes sense @mountiny, or do you have an idea what might be causing the frozen animation?

@mountiny

Copy link
Copy Markdown
Contributor Author

@TomatoToaster That makes a lot of sense, I would not block it on this issue for sure. I will address the NAB comments and ask for final review.

I do not have a clue if this is just caused by the emulator or it really is like that on physical Android device (I hope the app is not so slow on physical android device as it is in the emulator 😅)

@mountiny

Copy link
Copy Markdown
Contributor Author

@TomatoToaster I have updated the small changes, so asking for a final review. Thank you very much 🙌

@mountiny mountiny requested a review from TomatoToaster August 10, 2021 14:47

@TomatoToaster TomatoToaster left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, feel free to merge if you catch it passing the tests before I do.

@mountiny mountiny merged commit 49f002b into main Aug 10, 2021
@mountiny mountiny deleted the vit-addLoadingAnimation branch August 10, 2021 15:30
@isagoico

Copy link
Copy Markdown

@vitHoracek Hello! We currently don't have a test domain to QA this since all our test domains already have the expensify card and the "Get started" won't show for us. Can this be tested internally?

@mountiny

Copy link
Copy Markdown
Contributor Author

@isagoico Of course, sorry I have not added the label from the start! I will take care of it.

@MitchExpensify

Copy link
Copy Markdown
Contributor

Tested - Looks great! Good to check off the list @isagoico

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by @mountiny in version: 1.0.83-2 🚀

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

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by @isagoico in version: 1.0.85-9 🚀

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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants