Details

Reviewers

ashoat
ginsu
tomek

Commits

rCOMM3b38c3691379: [native] Update `DisconnectedBar` to display based on `state.connectivity.
rCOMM8f592ed33c85: [native] Update `DisconnectedBar` to display based on `state.connectivity.

Summary

Similar to D10469, but for native.

As mentioned in that diff, going to start by updating the DisconnectedBar on both native and web client to use network connectivity info instead of keyserver connectivity info.

Will then go ahead and rename eg useDisconnectedBarVisibilityHandler to make it clear that they're no longer connected to DisconnectedBar.

Depends on D10469

Test Plan

Open native app on physical device
Kill local keyserver
Observe that nothing happens
Turn off WiFi
Observe DisconnectedBar

Diff Detail

Repository

rCOMM Comm

Branch

arcpatch-D10475_1 (branched from master)

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Dec 28 2023, 12:50 AM

atul added inline comments.Dec 28 2023, 12:52 AM

native/navigation/disconnected-bar.react.js
61 ↗	(On Diff #35043)	CC @ashoat, similarly to https://phab.comm.dev/D10469#inline-63752 there's product decision to be made here on whether we want to show `DISCONNECTED` or `CONNECTING...`. Can easily change to `DISCONNECTED` if that'd be preferred.

Harbormaster completed remote builds in B25364: Diff 35043.Dec 28 2023, 1:08 AM

atul requested review of this revision.Dec 28 2023, 1:08 AM

atul added a child revision: D10476: [lib] Rip `useShouldShowDisconnectedBar` and `useDisconnectedBar` out of `hooks/disconnected-bar`.Dec 28 2023, 1:18 AM

inka added a subscriber: inka.Dec 28 2023, 1:34 AM

Would like one other person's perspective on the product question inline

native/navigation/disconnected-bar.react.js
26 ↗	(On Diff #35043)	Can we try to use the "native driver" so that the animation is more smooth? I remember trying this initially, but it didn't work at the time the component was built. Maybe that's changed. Docs: [1], [2], [3] If that doesn't work, we could potentially try Reanimated. Back in Reanimated 1 there was an issue preventing it from working, but hopefully Reanimated 2 fixes it. Converting to Reanimated is a more involved task, though... if that's necessary, it can probably be handled as a follow-up task.
61 ↗	(On Diff #35043)	Same opinion as in D10469. We could also consider using the `connecting` color, but the `disconnected` text

atul added inline comments.Dec 28 2023, 3:42 PM

native/navigation/disconnected-bar.react.js
61 ↗	(On Diff #35043)	I think "DISCONNECTED" text with `connecting` color is the best bet.

atul added inline comments.Dec 28 2023, 4:19 PM

native/navigation/disconnected-bar.react.js
26 ↗	(On Diff #35043)	Just to clarify, we'd set `useNativeDriver: true` in the call to `Animated.timing(...)` on lines 35-38 right? Just want to make sure there's no change that needs to happen here when we're setting `showingRef.current`