Loading indication for sync #232

ioana-farcas · 2018-11-23T21:42:57Z

Fixes #48

Testing and Review Notes

@mozilla-lockbox/ux
I can not tell if the bottom bar with the spinner has a shadow in zeplin...so I did not add one. Please let me know if it is needed.

Screenshots or Videos

To Do

add “WIP” to the PR title if pushing up but not complete nor ready for review
double check the original issue to confirm it is fully satisfied
add testing notes and screenshots in PR description to help guide reviewers
add unit tests
- optional: consider adding instrumentation (integration/UI) tests
consider running this branch in a debug simulator and check for memory leak notifications or any warnings
request the "UX" team perform a design review (if/when applicable)
make sure CI builds are passing (e.g.: fix lint and other errors)
check on the accessibility of any added UI

nickbrandt

@ioana-farcas this looks great! There is a shadow on the snackbar, though it's difficult to select the layer in Zeplin, so I attached an image below showing the values. Just a couple other things I noticed while testing:

Can we update the screen background to: #EDEDF0
The first time I sign in after downloading the build (making sure to delete any old versions first), it seems to get hung up during syncing and never displays my entries. I can navigate to Settings or Account and back and it still just continues to sync. If I force quit out of Lockbox, re-open, then sign in again - it syncs and then loads my entries as expected.
Note: I don't have this issue on the simulator, only on my Pixel device
Sounds like this is an issue with Sync and not tied to this PR 👍
Question: Are we syncing the entry list every time the user navigates back to the Entry List? I noticed that after my entries do show, and I navigate to either Settings, Account, or an Entry Detail, and then navigate back to the Entry List, it shows that it is syncing again. I was thinking that we only sync on signing in, and then on a time interval after that, but perhaps I'm mistaken on this. Sounds like this is being addressed in another issue 👍

Sync stalling video capture

devinreams · 2018-11-28T16:33:59Z

Some refactor required per @ioana-farcas and @jhugman...

related to but doesn't fully fix #262 will waiting on toast being built at #232

related to but doesn't complete #262 will wait on view and styles built at #232 for reuse

changecourse

I'm encountering some strange behavior on this release build 1679...

When I sign in with my personal account on a newly installed build, it works, but displays a 404 webpage for a brief moment between a successful authentication (via FxA) and the sync toaster loading up my entries. I have a random hunch that this might be the FxA "Account Confirmed" dialog that isn't being able to be fetched prior to transitioning to the entry list, though that's just a shot in the dark.

When I sign in with test data, I am unable to trigger the sync toaster... but when I disconnect from the test account and try to then log in with my personal account instead, I'm actually presented with the test account entries instead of my personal entries. (@sashei didn't we experience something similar on iOS at some point?)

FWIW, the sync indicator looks good, as does this background of the empty state... it's just now these two, potentially larger, external issues I'm experiencing.

sashei · 2018-12-17T16:59:56Z

@changecourse a few things:

the 404 page is expected for now, we aren't navigating away from the FxA redirect quickly enough for the time being and the expectation is that this will be fixed with the implementation of the onboarding stories (all onboarding stories reflect this expectation Enable biometric unlock (onboarding) #30 Onboarding confirmation with security theatre (onboarding) #24 If device pin/biometrics not set, provide instructions to do so (onboarding) #35 Enable autofill (onboarding) #55)
I have experienced similar issues switching from using test data to real data, I have to do a hard uninstall-reinstall cycle every time, so I don't think this is related to this PR necessarily. I haven't filed an issue for that but can do that today if others are having issues!!

changecourse · 2018-12-17T17:03:44Z

@sashei thanks for the clarifications! As long as we're tracking both of those, we're good then :)

UI-wise, we're good.

@changecourse

UI changes reviewed by @changecourse

devinreams · 2018-12-17T17:09:24Z

I have experienced similar issues switching from using test data to real data, I have to do a hard uninstall-reinstall cycle every time, so I don't think this is related to this PR necessarily. I haven't filed an issue for that but can do that today if others are having issues!!

Ah interesting, I haven't seen that myself but since I was here already, went ahead and filed that issue as a P2: #277

sashei

Looks great, thanks so much for your hard work on this PR @ioana-farcas :)) ♻️ 🎉 💯

sashei · 2018-12-17T17:14:06Z

app/src/main/java/mozilla/lockbox/store/DataStore.kt

@@ -48,7 +48,7 @@ open class DataStore(
    private val listSubject: BehaviorRelay<List<ServerPassword>> = BehaviorRelay.createDefault(emptyList())

    val state: Observable<State> get() = stateSubject
-    val syncState: Observable<SyncState> = PublishSubject.create()
+    open val syncState: Observable<SyncState> = ReplaySubject.create()


sashei · 2018-12-17T17:20:36Z

app/src/main/java/mozilla/lockbox/view/ItemListFragment.kt

+            showAndRemove(view!!.entriesView, view!!.loadingView)
+        }
+        view!!.filterButton.isClickable = !isLoading


This could be a follow-up improvement, but it would be nice for these isClickable and isEnabled settings to each be their own Consumers on the View rather than side effects of showing / hiding the loading spinner. IIRC from iOS, we have other situations (no saved entries comes to mind) where we want to disable the filter / sort buttons. However, I'm happy to let this PR merge as-is and do that work in later tickets :)

@sashei You're right. I will merge this and add the modifications in another PR. Thanks for the review!

Filed a follow-up issue here: #279

ioana-farcas requested a review from a team as a code owner November 23, 2018 21:42

ghost assigned ioana-farcas Nov 23, 2018

ghost added the in progress label Nov 23, 2018

ioana-farcas force-pushed the 48-loading-indication_sync branch 2 times, most recently from 69cff93 to c0f132f Compare November 27, 2018 17:08

ioana-farcas changed the title ~~[WIP] Loading indication for sync~~ Loading indication for sync Nov 27, 2018

ioana-farcas requested a review from a team November 27, 2018 18:41

nickbrandt previously requested changes Nov 27, 2018

View reviewed changes

ioana-farcas force-pushed the 48-loading-indication_sync branch from 6daa617 to 0398a82 Compare November 28, 2018 10:35

devinreams changed the title ~~Loading indication for sync~~ [WIP] Loading indication for sync Nov 28, 2018

ioana-farcas force-pushed the 48-loading-indication_sync branch 5 times, most recently from df17439 to a253d84 Compare December 12, 2018 15:33

devinreams added a commit that referenced this pull request Dec 12, 2018

add sane colors for copy toast notifications

cac0b10

related to but doesn't fully fix #262 will waiting on toast being built at #232

devinreams added a commit that referenced this pull request Dec 13, 2018

add sane colors for copy toast notifications, for now

da4bc1a

related to but doesn't complete #262 will wait on view and styles built at #232 for reuse

ioana-farcas force-pushed the 48-loading-indication_sync branch from a253d84 to bd5dd6d Compare December 13, 2018 15:48

devinreams mentioned this pull request Dec 13, 2018

Minor UI changes for item detail toast and learn more link colors #272

Merged

5 tasks

ioana-farcas force-pushed the 48-loading-indication_sync branch 2 times, most recently from b5a9bda to 1d8901e Compare December 17, 2018 11:09

ioana-farcas added 6 commits December 17, 2018 16:23

Loading indication for sync

ae1594b

Disable filter and sort button when loading

5a6631c

Fixed unit tests

0b5e287

Fixed lint

d389a33

Fixed UI tests

82f99f4

Small refactoring

a960fee

ioana-farcas force-pushed the 48-loading-indication_sync branch from 6350107 to 3181c68 Compare December 17, 2018 14:51

Added shadow

292d3c7

ioana-farcas force-pushed the 48-loading-indication_sync branch from 3181c68 to 292d3c7 Compare December 17, 2018 14:55

wip

116ed86

ioana-farcas force-pushed the 48-loading-indication_sync branch from ee98fc3 to 116ed86 Compare December 17, 2018 15:45

ioana-farcas changed the title ~~[WIP] Loading indication for sync~~ Loading indication for sync Dec 17, 2018

ioana-farcas requested a review from a team December 17, 2018 16:21

changecourse reviewed Dec 17, 2018

View reviewed changes

devinreams mentioned this pull request Dec 17, 2018

Switching from test data to real account still shows test data #277

Closed

changecourse approved these changes Dec 17, 2018

View reviewed changes

sashei approved these changes Dec 17, 2018

View reviewed changes

ioana-farcas merged commit 288817c into master Dec 17, 2018

ghost removed the in progress label Dec 17, 2018

ioana-farcas deleted the 48-loading-indication_sync branch December 17, 2018 17:36

This was referenced Dec 17, 2018

Change isClickable and isEnabled settings to each be their own Consumers #279

Open

Copy (toast) notifications styling #262

Closed

devinreams mentioned this pull request Jan 17, 2019

Syncing toast notification is missing after pull to refresh added #388

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loading indication for sync #232

Loading indication for sync #232

ioana-farcas commented Nov 23, 2018 •

edited

nickbrandt left a comment •

edited

devinreams commented Nov 28, 2018

changecourse left a comment

sashei commented Dec 17, 2018

changecourse commented Dec 17, 2018

devinreams commented Dec 17, 2018

sashei left a comment

sashei Dec 17, 2018

sashei Dec 17, 2018

ioana-farcas Dec 17, 2018

devinreams Dec 17, 2018

Loading indication for sync #232

Loading indication for sync #232

Conversation

ioana-farcas commented Nov 23, 2018 • edited

Testing and Review Notes

Screenshots or Videos

To Do

nickbrandt left a comment • edited

Choose a reason for hiding this comment

devinreams commented Nov 28, 2018

changecourse left a comment

Choose a reason for hiding this comment

sashei commented Dec 17, 2018

changecourse commented Dec 17, 2018

devinreams commented Dec 17, 2018

sashei left a comment

Choose a reason for hiding this comment

sashei Dec 17, 2018

Choose a reason for hiding this comment

sashei Dec 17, 2018

Choose a reason for hiding this comment

ioana-farcas Dec 17, 2018

Choose a reason for hiding this comment

devinreams Dec 17, 2018

Choose a reason for hiding this comment

ioana-farcas commented Nov 23, 2018 •

edited

nickbrandt left a comment •

edited