[web] Update `DisconnectedBar` to display based on `useNetworkConnected()`
ClosedPublic
Actions

Authored by atul on Dec 27 2023, 6:27 PM.

Details

Reviewers

ashoat
ginsu
tomek

Commits

rCOMM6ca82c09dbfa: [web] Update `DisconnectedBar` to display based on `useNetworkConnected()`

Summary

Part of https://linear.app/comm/issue/ENG-6137/modify-disconnectedbar-so-it-pulls-from-connectivityinfo.

To begin, I'm going to update the web and native components to skip eg useShouldShowDisconnectedBar, etc. and just get the boolean they need (is the network connected or not) "directly" from useNetworkConnected on web and (state => state.connectivity.connected) on native.

I'll then go ahead and rename all of eg useDisconnectedBarVisibilityHandler to make it clear that they're no longer connected to the DisconnectedBar. I think we'll still want all the Redux actions and whatnot for the "Keyserver status"? page that @ginsu is working on, so I won't rip them out.

Test Plan

Open local web app
Kill local keyserver
Observe that nothing happens:

Disconnect laptop from WiFi
Observe the DisconnectedBar:

Diff Detail

Repository

rCOMM Comm

Branch

arcpatch-D10469 (branched from master)

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Dec 27 2023, 6:27 PM

atul published this revision for review.Dec 27 2023, 6:33 PM

atul added inline comments.

web/redux/disconnected-bar.js
18 ↗	(On Diff #35036)	CC @ashoat there's a product decision to be made here. We'll no longer have separate "CONNECTING..." and "DISCONNECTED" states. Instead we'll have a `networkConnected: boolean` which doesn't differentiate between the two. When the client is disconnected should we show `CONNECTING...` or `DISCONNECTED`? I slightly prefer `CONNECTING...` since it seems less "intense," but don't have a strong preference. I can easily flip it to `DISCONNECTED` if that'd be preferred.

atul edited the test plan for this revision. (Show Details)Dec 27 2023, 6:40 PM

Harbormaster completed remote builds in B25357: Diff 35036.Dec 27 2023, 6:46 PM

atul added a child revision: D10475: [native] Update `DisconnectedBar` to display based on `state.connectivity.connected`.Dec 28 2023, 12:50 AM

atul mentioned this in D10475: [native] Update `DisconnectedBar` to display based on `state.connectivity.connected`.Dec 28 2023, 12:52 AM

atul edited the summary of this revision. (Show Details)

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

Would like one other person's perspective on the product question inline, but the code looks good

web/redux/disconnected-bar.js
18 ↗	(On Diff #35036)	I'm okay either way, but have a slight preference for `DISCONNECTED` since it feels more accurate. Curious what other reviewers think