-
Notifications
You must be signed in to change notification settings - Fork 984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Connection status info #6865
Connection status info #6865
Conversation
Pull Request Checklist
|
223e104
to
4c52636
Compare
the desktop profile tab
4c52636
to
d76fcdb
Compare
Thanks Rob! Can you include some screenshots in the issue? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rcullito ! Tested this locally, and turned WiFi on/off. It went from Connected with 5 peers, including mailserver
to Disconnected
and then Connected with 2,4,5 peers
. Looks good to me :)
Thanks for the thorough local testing @siphiuel ! 🙏 |
87% of end-end tests have passed
Failed tests (4)Click to expand
Passed tests (26)Click to expand |
@rcullito awesome work! |
fixes #6567
Summary
This is a basic port of the criteria laid out by @adambabik during our time in Prague and detailed in the above-linked issue "Show connection status within the app".
My first question for the clj folks is whether the various states of
:node/status
, which is already in app-db, can be a reasonable proxy fordiscovery.started
anddiscovery.stopped
.If not, I'd be curious about current examples within re-frame around how we can use json-rpc to get these values? I'm still ramping on the inner workings of chat but the principle form of message communication seems to be waiting for gethEvents via the signal-received event handler, and I wasn't sure how to go about querying values from the status-go side without merely waiting for these events.
One other question had to do with whether the whisper min and max here should both be set to 2, and what that means currently? https://github.com/status-im/status-react/blob/develop/src/status_im/node/core.cljs#L50 While this PR is pretty simple, we talked about a visual indicator something akin to a traffic light, and I'm thinking the whisper min and max will come into play here for "yellow", though of course it would be good to discuss this with those who have been around the part of the code base for a while already.
I chose the "advanced settings" tab for this PR because we are already displaying mailserver information there. It seems like a more visual indicator could be planned as a follow up to this in collaboration with @EugeOrtiz for design input. (as has been mentioned on the issue)
Testing instructions