Added sync indicator #2810

parasharrajat · 2021-05-11T21:52:16Z

Please review.

Details

Added sync indicator which is shown when ONYXKEYS.IS_LOADING_AFTER_RECONNECT is true

Fixed Issues

Fixes #2606

Tests

Web/Desktop

Open the JavaScript Dev console and go to the network tab.
Switch the app to offline (see demo video below with screenshots)
Switch the app back online. Verify that the indicator is shown.

QA Steps

Web/Desktop

Open the JavaScript Dev console and go to the network tab.
Switch the app to offline (see demo video below with screenshots)
Switch the app back online. Verify that the indicator is shown.

iOS/Android/mWeb

Put the app on background on mobile, then going back to foreground indicator should be shown
Put on Airplane mode, then turn it off, the indicator will be visible.

Tested On

Screenshots

Web

indicator-w.mp4

Mobile Web

1621372645191.mp4

Desktop

indicator-d.mp4

iOS

indicator-ios.mp4

Android

1621371423054.mp4

shawnborton · 2021-05-11T22:09:11Z

src/components/AvatarWithIndicator.js

+                        <Icon
+                            src={Sync}
+                            fill={themeColors.textReversed}
+                            width="100%"


Could you try making the icon 12x12? This way there is a little bit of margin around the icon inside of the 16x16 green circle.

@shawnborton But indicator size is 12x12 including 2px transparent border around. So Green circle is 8x8.

We want the indicator to become larger when we are showing that content is syncing. So the green online indicator should grow in size and then show the spinning icon inside of it. Let me know if that makes sense - happy to make a prototype as well.

OK after growing, it should be 16px. makes sense.

@shawnborton It looks a little big. Don't you think so?

Or if this is looking fine, let me know I will update the PR.

@shawnborton any thoughts?

I agree it looks big. I think ideally the size of the indicator wouldn't need to change – we just show the spinning icon in the normal-size indicator if it's loading.

We can try a smaller size, which would be a green circle of 12x12, with the icon inside of it at 8x8. It would look like this:

src/components/AvatarWithIndicator.js

src/pages/home/sidebar/SidebarLinks.js

src/components/AvatarWithIndicator.js

Co-authored-by: Amal Nazeem <amalnazeem@gmail.com>

TomatoToaster

LGTM - I'm might get someone to double check me. But I'd also wait for Shawn to get back to you on the icon size.

roryabraham · 2021-05-14T00:20:45Z

Added myself as a review to help buddy-check @TomatoToaster. @parasharrajat Will review tomorrow 👍

roryabraham · 2021-05-14T16:22:20Z

Hey @parasharrajat can you please include screenshots for the other platforms? Also, I edited the tests + qa steps to clarify them and add platform-specific instructions. Also, do you have any recommendations for testing this on an iOS simulator/Android emulator?

parasharrajat · 2021-05-14T16:24:19Z

Let me add screenshots for all devices. Thanks for updating the steps.

do you have any recommendations for testing this on an iOS simulator/Android emulator?

Currently not.

shawnborton · 2021-05-14T17:16:38Z

Apologies @parasharrajat as I was out of office yesterday. I'll review this soon!

roryabraham · 2021-05-14T17:13:21Z

src/components/AvatarWithIndicator.js

+        const indicatorStyles = [
+            styles.alignItemsCenter,
+            styles.justifyContentCenter,
+            {


We've tried to take a pretty hard stance against inline styles. In this scenario, we would usually make a function in styles.js called getAvatarIndicatorStyles or something and dynamically generate these styles in there. You can see how I handed transforms in a dynamic style function in src/styles/getTooltipSyles.js. We haven't really decided whether dynamic style generators should be in their own module or directly in styles.js, so it's up to you! Since it's probably just one function, I would probably put it directly in styles.js

roryabraham · 2021-05-14T17:17:37Z

src/components/AvatarWithIndicator.js

+                        <Icon
+                            src={Sync}
+                            fill={themeColors.textReversed}
+                            width="100%"


I agree it looks big. I think ideally the size of the indicator wouldn't need to change – we just show the spinning icon in the normal-size indicator if it's loading.

src/components/AvatarWithIndicator.js

roryabraham · 2021-05-14T17:40:42Z

@parasharrajat During my testing on iOS I was able to successfully test this the first time the app loads. However, repeating the same steps I am unable to get it to work again. Could this be related to needing shouldComponentUpdate?

FWIW I see three ways to test this locally on iOS:

(only Expensify employees) Set USE_NGROK=true in your .env to point the app at your VM, and then turn ngrok off and back on to simulate the loss and regain of a network connection.
(iOS Simulator) Follow these instructions to set up Network Link Conditioner.
Run the app on a physical device and turn airplane mode on and off.

roryabraham · 2021-05-14T17:44:11Z

Looking at my ngrok logs it seems like the problem is like this:

I turn off ngrok, so the app registers that it's offline and starts logging many "possible unhandled promise rejection" warnings.
I turn ngrok back on, but it doesn't notice and start clearing the network queue again. So the loading indicator doesn't show.

parasharrajat · 2021-05-14T17:47:40Z

@roryabraham Sorry. I just found something is off but yet to find where exactly. Animation does not seem to Loop well. Please hold your testing until I push a fix to that.

I see successful state change without shouldComponentUpdate as In absence of this by default shallow matching will be done for props and thus it will work without adding custom shouldComponentUpdate.
I see succesful value change on ComponentDidUpdate of isSyncing.

parasharrajat · 2021-05-18T19:11:00Z

@shawnborton Does this look right?

indicator-w.mp4

shawnborton · 2021-05-18T19:15:36Z

This looks great Rajat! Could you just slightly speed up the speed of the icon rotating? Otherwise I think it's good to go.

parasharrajat · 2021-05-18T21:19:21Z

Ready for review.

shawnborton · 2021-05-18T21:24:02Z

src/components/AvatarWithIndicator.js

+                        <Icon
+                            src={Sync}
+                            fill={themeColors.textReversed}
+                            width={6}


Just to make sure I am following, when the indicator is in syncing mode, the small white icon that spins is shown at 6x6?

I am not setting the width & height for the indicator. Instead, I use scaling which is Smooth. So when we scale the green dot, all its children including the icon will scale. 8px icon was looking big when scaling so I have adjusted it and found 6px is looking fine.

Scaling using fixed px is not smooth as the indicator is absolutely placed thus when we increase width & height, It changes location and does not look good. Oppose to what scaling keeps the center and scale uniformly.

https://github.com/parasharrajat/Expensify.cash/blob/8aca3f289a94218ea0c0f0fc0a24749b15fc87bf/src/styles/getAvatarWithIndicatorStyles.js#L10

Ah, that makes sense to me and sounds good!

roryabraham · 2021-05-18T21:58:58Z

@parasharrajat When testing this on iOS I was able to get it in a state where it is "stuck" syncing. All I did was turn off wifi and then turn it back on using the Network Link Conditioner tool.

parasharrajat · 2021-05-18T22:07:29Z

Ok. Let me check it with Network Link Conditioner.

parasharrajat · 2021-05-19T15:38:25Z

@roryabraham Not sure how to turn off the WIfi with NLC. I would definitely need that tip but I tried 100% loss profile and On bringing the app on foreground from background, I also saw that syncing never stops~~(takes like 5 mins). I checked the logs it seems that ONYXKEYS.IS_LOADING_AFTER_RECONNECT state is not updating in the Onyx thus stop is never called afterward. This logic completely relies on ONYXKEYS.IS_LOADING_AFTER_RECONNECT key.

Update

for some cases, ONYXKEYS.IS_LOADING_AFTER_RECONNECT is set to false but mostly it is never set to false after triggering syncing on 100% loss profile.

roryabraham · 2021-05-19T16:50:50Z

Not sure how to turn off the WIfi with NLC. I would definitely need that tip but I tried 100% loss profile

Yeah, this is what I meant by "turn off Wifi". Sorry it that wasn't clear.

This logic completely relies on ONYXKEYS.IS_LOADING_AFTER_RECONNECT key.

I think that since this PR introduces a visual indication of the state of that key, we should not merge it until the key is being successfully updated when we expect it to.

parasharrajat · 2021-05-19T16:52:54Z

I think that since this PR introduces a visual indication of the state of that key, we should not merge it until the key is being successfully updated when we expect it to.

Agree, let me see what happens with the ONYXKEYS.IS_LOADING_AFTER_RECONNECT Key in that state.

parasharrajat · 2021-05-19T17:20:30Z

@roryabraham
Strange, now it seems to work fine. I checked the Triggering logic and we are using Promises.all So it is possible that some of the API calls hanged forever which is unlikely. It should not happen with real devices even if there is a very poor connection. The only thing I can think of at the moment is Timeout the calls.

roryabraham · 2021-05-21T00:47:30Z

@parasharrajat Sorry, I still can't seem to get this working on iOS. 😕 It seems to spin forever, regardless of the state of the network connection. Can you provide a screencast of it working on iOS? The code seems fine, so it could be something weird with ngrok, or my VM or something. I don't want to block you on this forever, but I also can't approve this without seeing it work on all platforms. If you can provide a screencast of this working on iOS I might be willing to just merge this and see if it works on staging.

parasharrajat · 2021-05-21T00:48:56Z

Sure @roryabraham. I will share the Screencast.

roryabraham · 2021-05-21T00:51:10Z

@TomatoToaster Did this work for you?

parasharrajat · 2021-05-21T17:25:53Z

@roryabraham Here is a recording. Its little Blurry due to resolution but functionality can be seen well.

indicator-ios.mp4

roryabraham

Okay, looks good to me. Thanks for providing screen recordings on all platforms.

roryabraham · 2021-05-21T17:41:16Z

@shawnborton Code looks good to me. I'll let you to a final design review, then if everything looks good feel free to merge once all the checks pass.

shawnborton · 2021-05-21T17:43:10Z

Looks good to me design-wise!

OSBotify · 2021-05-21T19:18:56Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

parasharrajat added 2 commits May 12, 2021 03:15

new: added sync indicator

ec80ff2

animation on native

e37ee15

parasharrajat requested a review from a team as a code owner May 11, 2021 21:52

MelvinBot requested review from TomatoToaster and removed request for a team May 11, 2021 21:52

typo

ae3858c

shawnborton reviewed May 11, 2021

View reviewed changes

TomatoToaster reviewed May 13, 2021

View reviewed changes

src/components/AvatarWithIndicator.js Show resolved Hide resolved

TomatoToaster suggested changes May 13, 2021

View reviewed changes

src/pages/home/sidebar/SidebarLinks.js Outdated Show resolved Hide resolved

src/components/AvatarWithIndicator.js Outdated Show resolved Hide resolved

parasharrajat and others added 2 commits May 14, 2021 05:03

Update src/pages/home/sidebar/SidebarLinks.js

a3baacd

Co-authored-by: Amal Nazeem <amalnazeem@gmail.com>

Update src/components/AvatarWithIndicator.js

78a084b

Co-authored-by: Amal Nazeem <amalnazeem@gmail.com>

TomatoToaster previously approved these changes May 14, 2021

View reviewed changes

roryabraham self-requested a review May 14, 2021 00:19

roryabraham assigned parasharrajat May 14, 2021

Pull latest changes

fcf5b8a

roryabraham requested changes May 14, 2021

View reviewed changes

parasharrajat changed the title ~~Added sync indicator~~ [WIP] Added sync indicator May 14, 2021

parasharrajat added 2 commits May 15, 2021 02:14

fix: animation issue with indicator

fa3ae71

Pull remote changes

743ee68

parasharrajat dismissed TomatoToaster’s stale review via 743ee68 May 14, 2021 20:46

parasharrajat added 2 commits May 17, 2021 02:34

pull latest changes

4ebe4be

fix: sync icon is visible after user is offline

293375b

chore: animation timing

8aca3f2

parasharrajat changed the title ~~[WIP] Added sync indicator~~ Added sync indicator May 18, 2021

parasharrajat requested a review from roryabraham May 18, 2021 21:21

shawnborton reviewed May 18, 2021

View reviewed changes

TomatoToaster previously approved these changes May 18, 2021

View reviewed changes

merge Conflicts

92addf9

parasharrajat dismissed TomatoToaster’s stale review via 92addf9 May 21, 2021 17:30

roryabraham approved these changes May 21, 2021

View reviewed changes

parasharrajat requested a review from TomatoToaster May 21, 2021 17:41

shawnborton approved these changes May 21, 2021

View reviewed changes

roryabraham merged commit fc2d0e1 into Expensify:main May 21, 2021

OSBotify mentioned this pull request May 22, 2021

Deploy Checklist: Expensify.cash 2021-05-22 #3076

Closed

6 tasks

Added sync indicator #2810

Added sync indicator #2810

Conversation

parasharrajat commented May 11, 2021 • edited Loading

Details

Fixed Issues

Tests

Web/Desktop

QA Steps

Web/Desktop

iOS/Android/mWeb

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

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

parasharrajat May 13, 2021 • edited Loading

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

TomatoToaster left a comment

Choose a reason for hiding this comment

roryabraham commented May 14, 2021

roryabraham commented May 14, 2021

parasharrajat commented May 14, 2021 • edited Loading

shawnborton commented May 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roryabraham commented May 14, 2021

roryabraham commented May 14, 2021

parasharrajat commented May 14, 2021 • edited Loading

parasharrajat commented May 18, 2021

shawnborton commented May 18, 2021

parasharrajat commented May 18, 2021

Choose a reason for hiding this comment

parasharrajat May 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roryabraham commented May 18, 2021 • edited Loading

parasharrajat commented May 18, 2021

parasharrajat commented May 19, 2021 • edited Loading

Update

roryabraham commented May 19, 2021

parasharrajat commented May 19, 2021

parasharrajat commented May 19, 2021 • edited Loading

roryabraham commented May 21, 2021

parasharrajat commented May 21, 2021

roryabraham commented May 21, 2021

parasharrajat commented May 21, 2021

roryabraham left a comment

Choose a reason for hiding this comment

roryabraham commented May 21, 2021

shawnborton commented May 21, 2021

OSBotify commented May 21, 2021

parasharrajat commented May 11, 2021 •

edited

Loading

parasharrajat May 13, 2021 •

edited

Loading

parasharrajat commented May 14, 2021 •

edited

Loading

parasharrajat commented May 14, 2021 •

edited

Loading

parasharrajat May 18, 2021 •

edited

Loading

roryabraham commented May 18, 2021 •

edited

Loading

parasharrajat commented May 19, 2021 •

edited

Loading

parasharrajat commented May 19, 2021 •

edited

Loading