implemented a connection status indicator

[Glubs9]

4 is the issue this pull request tries to solve

[OliverBalfour]

Looks like if the IP address is correct when you load the page, and you hit backspace, it stays as "Connected". I think this is a re-rendering issue - the safety panel doesn't know to re-render when the socket status changes because the socket status isn't part of any state or props.

I think the way to fix this is instead of using that.socket we should pass a status={this.socket.readyState} prop to the safety panel. In fact, that feels dirty to me already, I think we should have a prop for each property of that we use instead - could you do this in a small second commit?

Also, I think what we want to display is simply "Connected" (ready state 1) or "Disconnected" (any other ready state). We've got reconnection logic that should make the "Closed" and "Closing" states very transient, and "Connecting" and "Connected" look very similar visually.

[Glubs9]

I don't think it is wise to change that to be a prop. I did misuse it in the original commit but looking at the only place it is used (line 67-69) it seems to be appropriate. Making use of the object to make it clear how it influences state, rather than passing arbitrary callbacks.

It is you're call though.

[OliverBalfour]

You can leave that there if you like for the existing use case. The current use in 67-69 is harmless, but the reason I suggested removing it is that it can be used in a dangerous way, so removing it makes it harder to accidentally do something dangerous. For instance:

• someone might change the name of the connect function in the other file expecting that it won't be called outside of that file (generally encapsulation is done like this in React unless you explicitly pass a function down as a prop)
• someone might use that.propertyOutsideOfState again in the render function, which will cause the same bug as we've just seen here

Also, looks like you've forgotten to edit / push the change where you give the <SafetyPanel /> a sockStatus prop!

1. When you give it this prop, make sure that the safety panel re-renders when the socket status changes. If the socket status is outside of the state, I don't think it will trigger a re-render. Maybe you need an event listener on the web socket that calls setState({ sockStatus: ... }) when the socket state changes?
2. Make sure to test the branch, especially under wacky conditions. In this case spamming refresh and deleting/editing the IP a lot and making sure the connection status is always in sync would reveal a couple bugs before reivew

[OliverBalfour]

@Glubs9 I've just pushed some edits (fixing the issue I mentioned above with re-renders not being triggered, eg if you deleted a single character of the IP it wouldn't say "Disconnected")

Merging now

[Glubs9]

gosh i'm really sorry. I had the fix written I just forgot to push it. I've been busy moving, I very much apologize

[OliverBalfour]

All good, take it easy!