Skip to content
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

API connection status notifications #1237

Merged
merged 13 commits into from
May 14, 2020
Merged

API connection status notifications #1237

merged 13 commits into from
May 14, 2020

Conversation

alexcjohnson
Copy link
Collaborator

@alexcjohnson alexcjohnson commented May 9, 2020

Closes #920 - turn _reload-hash fetch failures into a status indicator rather than recurring errors.

When the back end is connected and the debug menu is closed there are no extra indicators, but an indicator appears where the error count would be if disconnected:
Screen Shot 2020-05-09 at 2 54 21 AM
And next to the error count if there are errors too:
Screen Shot 2020-05-09 at 2 39 05 AM
With the debug menu open, we show both connected and disconnected states:
Screen Shot 2020-05-09 at 2 37 35 AM
Screen Shot 2020-05-09 at 2 37 58 AM

Styling could use some work - I feel like the small 🚫 icon is pretty good, but the large 🚫 and ✅ are a bit ugly and out of place with the rest of the debug menu, probably we want dedicated svg icons for this purpose?

I also put the connection status in the heading of the error display:
Screen Shot 2020-05-09 at 2 39 23 AM
Note that callbacks that fail to fetch still show up in the error log, because these go through their own fetch call.
Screen Shot 2020-05-09 at 2 49 08 AM
I think this makes sense, but it raise the question whether we should do something different if hot reloading is disabled but the error UI is enabled. Callback fetch failures don't currently show as backEndConnected: false, but without hot reloading to provide a heartbeat any "connected" status would be out of date most of the time. Maybe without hot reloading we just shouldn't show connection status at all?

Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
    • Decide on styling
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR
  • I have added entry in the CHANGELOG.md

@rpkyle
Copy link
Contributor

rpkyle commented May 9, 2020

@alexcjohnson I love the idea of capturing the state of the backend connection in an unobtrusive way, rather than silencing the notifications altogether.

I think this makes sense, but it raise the question whether we should do something different if hot reloading is disabled but the error UI is enabled. Callback fetch failures don't currently show as backEndConnected: false, but without hot reloading to provide a heartbeat any "connected" status would be out of date most of the time. Maybe without hot reloading we just shouldn't show connection status at all?

I'd be inclined to either silence it when hot reloading is disabled, or consider adding a parameter to the the dev tools UI set of options which disables the "heartbeat" upon request (keeping it active if debug mode is on).

@nicolaskruchten
Copy link
Contributor

Do we want to use “connected” or “reachable” or “available” or something like that instead? I want to avoid confusing people into thinking there is a live “connection” like a web socket or whatever.

Also re “backend” should we talk about “hot reload” instead?

@alexcjohnson
Copy link
Collaborator Author

Good point @nicolaskruchten - also as we've discussed "back end" doesn't mean the same thing to everyone. But I don't think we want to just describe it as about "hot reload" as then people may not connect it to "the server has crashed and needs to be restarted"

How about "server available / unavailable"?

@alexcjohnson
Copy link
Collaborator Author

Updated styling - with help from @angeladaodao 🎉
All happy:
Screen Shot 2020-05-12 at 3 04 27 AM
With tooltip on hover:
Screen Shot 2020-05-12 at 3 07 30 AM
Failure (while hovering):
Screen Shot 2020-05-12 at 3 05 09 AM
Hot reload disabled:
Screen Shot 2020-05-12 at 3 05 52 AM
Error box, showing status in the header (ONLY when unavailable), an updated error message when a callback tries to reach a dead server, and a regular error:
Screen Shot 2020-05-12 at 3 11 36 AM

@nicolaskruchten
Copy link
Contributor

Feedback on the new images: the check and cross look too similar in my view... They do or indicate very different things so they should look different. We have 4 things there now, two of which pop things up (callback graph, error list), two of which indicate something (error list, backend-status) and one of which closes the panel. The close button should probably look quite different from the other three, and maybe the backend-status can be the same size as the other two?

@alexcjohnson
Copy link
Collaborator Author

The new one is just an indicator, not a toggle button like the callback graph and errors. And the close button for that matter, which toggles the menu itself, so if anything it seems like the new status indicator is the one odd one out. Now I'm thinking of maybe having the blue <> stick around when the debug menu appears, and just have the debug menu grow out of it with the three pieces (callback graph, errors, and status indicator) appearing. Then the blue <> can be both the open and the close button. Maybe it rotates when the menu appears? I'll fiddle...

@alexcjohnson
Copy link
Collaborator Author

A little more compact perhaps?
Screen Shot 2020-05-12 at 5 27 31 PM

@nicolaskruchten
Copy link
Contributor

Now it looks odd to have a small one... I say make it the same size and maybe pop up a little explanation on click instead of just on hover?

@alexcjohnson
Copy link
Collaborator Author

latest variant - all icons the same size, turned the deselected icons grey and removed the border, some light open/close animation
debugmenu

@nicolaskruchten
Copy link
Contributor

I like it a lot! Maybe some text on the new icon like “Server” to match the others?

@alexcjohnson
Copy link
Collaborator Author

Yeah that's better - also switched back to X for server unavailable, since we don't have another x now and the circle can't be centered with the label. Anything else? Any code comments??
Screen Shot 2020-05-13 at 12 54 08 PM
Screen Shot 2020-05-13 at 12 53 45 PM
Screen Shot 2020-05-13 at 12 54 46 PM

Copy link
Member

@chriddyp chriddyp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃 beautiful!

@alexcjohnson alexcjohnson merged commit 79c6f30 into dev May 14, 2020
@alexcjohnson alexcjohnson deleted the connection-status branch May 14, 2020 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

special handling for _reload-hash in dev tools
5 participants