Feature: add loading indicator when ReconnectApp is running #48772

nkdengineer · 2024-09-09T03:48:35Z

Details

Fixed Issues

$ #46611
PROPOSAL: #46611 (comment)

Tests

Login to app
Go to offline mode
Back to online mode
Verify that: loading indicator is displayed

Verify that no errors appear in the JS console

Offline tests

QA Steps

Login to app
Go to offline mode
Back to online mode
Verify that: loading indicator is displayed

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: mWeb Chrome
- iOS: Native
- iOS: mWeb 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 the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- 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 grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- 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 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(theme.componentBG))
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
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 the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
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.

Screenshots/Videos

Android: Native

android.mov

Android: mWeb Chrome

android-mweb.mov

iOS: Native

ios.mov

iOS: mWeb Safari

ios-mweb.mov

MacOS: Chrome / Safari

web.mov

MacOS: Desktop

desktop.mov

melvin-bot · 2024-09-09T09:28:33Z

@getusha 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]

dubielzyk-expensify · 2024-09-09T09:42:26Z

We should be using our color variables here so on light mode we should end up with this:

It looks like the background of the loading indicator is a dark mode color?

nkdengineer · 2024-09-09T09:57:24Z

@dubielzyk-expensify I updated to fix this case.

dubielzyk-expensify · 2024-09-09T10:27:03Z

Awesome. Can you update the recordings please 😇?

nkdengineer · 2024-09-09T10:29:58Z

@dubielzyk-expensify i'll update soon

nkdengineer · 2024-09-09T10:54:16Z

@dubielzyk-expensify @getusha i updated, please check again

dannymcclain · 2024-09-09T13:22:18Z

I am really excited for this to be in the product 😊

dubielzyk-expensify · 2024-09-10T01:10:20Z

This is looking awesome. Tiny tiny nitpick, but do the background of the loading bar and the loading bar itself have rounded corners? If so, can we remove them? It looks a bit better when this is square to match the page header border

src/components/ProgressBar.tsx

nkdengineer · 2024-09-10T15:08:36Z

@getusha i updated

getusha

minor comments

getusha · 2024-09-15T10:05:50Z

src/components/ProgressBar.tsx

+                        withTiming(0, {duration: CONST.ANIMATED_PROGRESS_BAR_DURATION, easing: Easing.bezier(0.65, 0, 0.35, 1)}),
+                        withTiming(100, {duration: CONST.ANIMATED_PROGRESS_BAR_DURATION, easing: Easing.bezier(0.65, 0, 0.35, 1)}),
+                    ),
+                    -1,


should we add this to const as well?

getusha · 2024-09-15T10:06:38Z

src/components/ProgressBar.tsx

+                    withSequence(
+                        withTiming(0, {duration: 0}),
+                        withTiming(0, {duration: CONST.ANIMATED_PROGRESS_BAR_DURATION, easing: Easing.bezier(0.65, 0, 0.35, 1)}),
+                        withTiming(100, {duration: CONST.ANIMATED_PROGRESS_BAR_DURATION, easing: Easing.bezier(0.65, 0, 0.35, 1)}),


@getusha We didn't add it in the const with the parameters of Easing.bezier like here

friendly bump @getusha

getusha · 2024-09-27T07:49:06Z

@nkdengineer the UI is still jumpy, didn't we position it absolute?

Screen.Recording.2024-09-27.at.10.48.14.in.the.morning.mov

getusha · 2024-09-27T09:50:33Z

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: mWeb Chrome
- iOS: Native
- iOS: mWeb 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 the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- 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 grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- 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(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
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 the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
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

Android: Native

Android: mWeb Chrome

Screen.Recording.2024-10-01.at.12.20.56.in.the.afternoon.mov

iOS: Native

iOS: mWeb Safari

Screen.Recording.2024-09-27.at.11.04.27.in.the.morning.mov

MacOS: Chrome / Safari

Screen.Recording.2024-09-27.at.11.00.26.in.the.morning.mov

MacOS: Desktop

nkdengineer · 2024-09-30T09:02:43Z

@nkdengineer the UI is still jumpy, didn't we position it absolute?

@getusha I fixed, please check again

getusha · 2024-10-01T09:27:08Z

src/components/ProgressBar.tsx

+
+    const animatedContainerStyle = useAnimatedStyle(() => {
+        return {
+            opacity: opacity.value,


Any way to hide the container? i can see it with a white background when it isn't active

From this comment, Do I need to fix this?

I think so yes, looks off to leave it like that. WDYT @dubielzyk-expensify?

@nkdengineer have we covered this?

dubielzyk-expensify · 2024-10-01T09:58:49Z

This is looking great! All 👍 from me on the visual end. Love the entry and exit fade too

getusha · 2024-10-01T10:06:24Z

Not sure if this is related but the performance on native is really bad it's nearly unusable, i'll check if this is any different on main

Screen.Recording.2024-10-01.at.1.04.58.in.the.afternoon.mov

srikarparsi · 2024-10-20T19:08:43Z

I'm not experiencing this problem on native iOS. @getusha could you please check if you have a chance and if not we can merge this

getusha · 2024-10-21T06:08:19Z

@srikarparsi could you add my device to the provisioning profile?
I am not able to install the Adhoc build on my phone

srikarparsi · 2024-10-22T01:53:33Z

Hey @getusha, we have a limited number of devices we can add and we're quite on the edge so I asked @mountiny if he could test. He saw some weird behavior so I wanted to ask, @nkdengineer, can you please merge main and I'll generate a new build and test?

nkdengineer · 2024-10-22T03:49:56Z

@srikarparsi merged main, please move forward

github-actions · 2024-10-22T04:43:50Z

🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪

Android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/48772/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/48772/index.html

Desktop 💻	Web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/48772/NewExpensify.dmg	https://48772.pr-testing.expensify.com

👀 View the workflow run that generated this build 👀

shawnborton · 2024-10-22T06:42:00Z

Hmm I keep getting this on the test build, any ideas?

srikarparsi · 2024-10-22T08:05:31Z

Yeah, it looks like we introduced a problem with login which is being looked at here. (Slack thread). Once it's fixed, we'll have to merge main again and spin up a new AdHoc build. Once this PR gets merged I'll move forward with that.

srikarparsi · 2024-10-22T08:07:23Z

In the meanwhile @shawnborton you can login with a new account (that hasn't been created before) and should be able to test it out.

srikarparsi · 2024-10-22T08:48:02Z

Hi @nkdengineer, do you think you could merge main one more time as the sign in bug got merged here?

mountiny · 2024-10-22T09:43:55Z

works fine for me, but the indicator is on the Report page too, I assume that should not be the case, right?

dannymcclain · 2024-10-22T13:54:58Z

works fine for me, but the indicator is on the Report page too, I assume that should not be the case, right?

I believe this is expected if ReconnectApp is indeed still running.

nkdengineer · 2024-10-22T16:39:11Z

@srikarparsi I merged.

mountiny · 2024-10-23T11:54:53Z

I believe this is expected if ReconnectApp is indeed still running.

I am not sure, I feel like its misleading as the OpenReport is what is loading all the details for the report so if anything I would expect the loader to indicate that the OpenReport is running on that screen.

dannymcclain · 2024-10-23T13:21:12Z

I am not sure, I feel like its misleading as the OpenReport is what is loading all the details for the report so if anything I would expect the loader to indicate that the OpenReport is running on that screen.

I guess I can't really speak to the technical side of it, but from the linked issue, this is what we decided:

On larger screens, display it only on the LHN, and on smaller screens, display it in both the LHN and the content pane (since the content pane takes up the whole screen on mobile)

srikarparsi · 2024-10-24T00:27:44Z

Yeah, I'm not really sure but kind of agree with what @dannymcclain said here

I also generated a new AdHoc build https://github.com/Expensify/App/actions/runs/11490278696

github-actions · 2024-10-24T00:47:43Z

🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪

Android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/48772/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/48772/index.html

Desktop 💻	Web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/48772/NewExpensify.dmg	https://48772.pr-testing.expensify.com

👀 View the workflow run that generated this build 👀

mountiny · 2024-10-24T03:54:04Z

No problem, I do not want to block on this, I just wanted to note that it was kind of confusing for me. Essentially you are on the Report screen, you see that something is loading but when it stops, nothing really changes for you (as it was not loading the specific report data necessarily)

dannymcclain · 2024-10-24T13:46:28Z

No problem, I do not want to block on this, I just wanted to note that it was kind of confusing for me. Essentially you are on the Report screen, you see that something is loading but when it stops, nothing really changes for you (as it was not loading the specific report data necessarily)

Oh dang. Maybe I have been misunderstanding this from the start. So are you saying that ReconnectApp may or may not be loading data for the specific report you're on? In your comment you said nothing really changed for you, but is it possible that something might change based on ReconnectApp running/finishing? Just trying to figure out if it makes sense to keep it on the report screen or not. If it's NEVER going to impact what you see on a report then maybe it's not... But if it might, then it probably is.

Would love some more thoughts from @Expensify/design. I'm worried that I haven't quite understood what ReconnectApp is actually doing all this time 😅

(I also don't want to block on this because I think this is a hugely valuable improvement to the app—but we can always make follow-ups if we need to)

shawnborton · 2024-10-24T16:05:17Z

you see that something is loading but when it stops, nothing really changes for you (as it was not loading the specific report data necessarily)

I guess I would view this as the app is checking to see if anything has changed, and it very well could be that nothing has changed but the app needs to think about it and check first. So I don't find this too weird personally I guess? But I also don't feel too strongly, even just having this on the LHN only is a big improvement.

dannymcclain · 2024-10-24T16:31:30Z

I guess I would view this as the app is checking to see if anything has changed, and it very well could be that nothing has changed but the app needs to think about it and check first. So I don't find this too weird personally I guess?

Yeah that makes sense to me.

But I also don't feel too strongly, even just having this on the LHN only is a big improvement.

In the interest of getting this in the product so we can see how it feels, I'd be down to just proceed how it's been implemented thus far. If we find that it DOES feel super strange, we could come back to it?

dubielzyk-expensify · 2024-10-24T23:56:10Z

Yeah I don't have much to add other than that I agree with all viewpoints here. I guess the only thing is that I think we should get this into the product, cause I think that we need to see how this performs in real life scenario and over some time before making adjustments. I'm happy to do LHN only though, but agree with Shawn that it's more of a "check in" and that things changing or not isn't really that important

srikarparsi · 2024-10-25T23:24:13Z

Cool, @getusha do you think you can finish reviewing this whenever you have a chance? If iOS native is still performing slow for you, I think we can merge without it because I just tested on my device with this build and it seems to be working fine.

getusha · 2024-10-28T08:05:58Z

I'll try wrapping this up today

srikarparsi · 2024-11-01T05:55:15Z

Friendly bump on the above @getusha, also there's conflicts now @nkdengineer

nkdengineer · 2024-11-01T08:14:56Z

Resolved conflict

getusha · 2024-11-01T09:53:30Z

@nkdengineer https://github.com/Expensify/App/pull/48772/files#r1818536847

nkdengineer added 2 commits September 9, 2024 10:46

Feature: add loading indicator when ReconnectApp is running

890eba7

fix ts check

7a77276

nkdengineer marked this pull request as ready for review September 9, 2024 09:28

nkdengineer requested review from a team as code owners September 9, 2024 09:28

melvin-bot bot requested review from getusha and removed request for a team September 9, 2024 09:28

fix style of progress bar

0265bc6

fix style progress bar

af9b5b8

getusha reviewed Sep 10, 2024

View reviewed changes

src/components/ProgressBar.tsx Show resolved Hide resolved

add new const and fix style progress bar

c177829

getusha reviewed Sep 15, 2024

View reviewed changes

merge main

2fcd466

nkdengineer added 2 commits September 30, 2024 15:32

Merge branch 'main' into feature/46611

57c403d

fix style progress bar

32117b2

getusha reviewed Oct 1, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

Merge branch 'main' into feature/46611

ac9dc23

Merge branch 'main' into feature/46611

0012f0e

resolve conflict

7f86ff7

Feature: add loading indicator when ReconnectApp is running #48772

Are you sure you want to change the base?

Feature: add loading indicator when ReconnectApp is running #48772

Conversation

nkdengineer commented Sep 9, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Sep 9, 2024

dubielzyk-expensify commented Sep 9, 2024

nkdengineer commented Sep 9, 2024

dubielzyk-expensify commented Sep 9, 2024

nkdengineer commented Sep 9, 2024

nkdengineer commented Sep 9, 2024

dannymcclain commented Sep 9, 2024

dubielzyk-expensify commented Sep 10, 2024

nkdengineer commented Sep 10, 2024

getusha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

getusha commented Sep 27, 2024

getusha commented Sep 27, 2024 • edited Loading

Reviewer Checklist

Screenshots/Videos

nkdengineer commented Sep 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dubielzyk-expensify commented Oct 1, 2024

getusha commented Oct 1, 2024

This comment has been minimized.

srikarparsi commented Oct 20, 2024

getusha commented Oct 21, 2024

srikarparsi commented Oct 22, 2024

nkdengineer commented Oct 22, 2024

github-actions bot commented Oct 22, 2024

shawnborton commented Oct 22, 2024

srikarparsi commented Oct 22, 2024

srikarparsi commented Oct 22, 2024

srikarparsi commented Oct 22, 2024

mountiny commented Oct 22, 2024

dannymcclain commented Oct 22, 2024

nkdengineer commented Oct 22, 2024

mountiny commented Oct 23, 2024

dannymcclain commented Oct 23, 2024

srikarparsi commented Oct 24, 2024

github-actions bot commented Oct 24, 2024

mountiny commented Oct 24, 2024

dannymcclain commented Oct 24, 2024

shawnborton commented Oct 24, 2024

dannymcclain commented Oct 24, 2024

dubielzyk-expensify commented Oct 24, 2024

srikarparsi commented Oct 25, 2024

getusha commented Oct 28, 2024

srikarparsi commented Nov 1, 2024

nkdengineer commented Nov 1, 2024

getusha commented Nov 1, 2024

nkdengineer commented Sep 9, 2024 •

edited

Loading

getusha commented Sep 27, 2024 •

edited

Loading