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

Pause Button #1106

Merged
merged 2 commits into from
Mar 7, 2016
Merged

Pause Button #1106

merged 2 commits into from
Mar 7, 2016

Conversation

davkal
Copy link
Contributor

@davkal davkal commented Mar 3, 2016

  • pauses topology and details panel updates
  • buffers requests
  • un-pause plays back the buffered requests
  • automatically un-pauses when topology is changed, or other node is
    clicked (then discards buffer)

Pause button

Fixes #1025

@peterbourgon
Copy link
Contributor

If state == paused, button should be play triangle, right?

@errordeveloper
Copy link
Contributor

This is really great!! The only thing I'm not too sure about is resume-on-click... When posting #1025, the use-case I had in mind was about exploring details of several nodes without having them move around, and potentially observe state of ephemeral nodes, as those naturally attract my attention.

@davkal
Copy link
Contributor Author

davkal commented Mar 3, 2016

If state == paused, button should be play triangle, right?

I prefer active/inactive pause. In the active state (paused) it blinks. This has the benefit of drawing attention to it in case the user wonders why nothing is updating. A blinking play button in a paused state would be misleading.

@2opremio
Copy link
Contributor

2opremio commented Mar 3, 2016

Awesome!

I however think we should make the Pause state more predominant, otherwise:

  1. User clicks pause (intentionally or by error)
  2. User forgets about it
  3. User sees that the UI is frozen and gets frustrated, blaming the software.
  4. User reports bug (or even worse, user does a permanent stop :) )

@davkal
Copy link
Contributor Author

davkal commented Mar 3, 2016

the use-case I had in mind was about exploring details of several nodes without having them move around

I see, essentially pausing the topology, but keeping the freedom to explore nodes. Is that more desirable than the current implementation (freezing topology and details to show them to someone)?

and potentially observe state of ephemeral nodes, as those naturally attract my attention.

This is inherently impossible/difficult to manage in the UI because the nodes may be gone by the time you explore them at which point in time a request for details would come back empty.

@davkal
Copy link
Contributor Author

davkal commented Mar 3, 2016

I however think we should make the Pause state more predominant, otherwise:

User clicks pause (intentionally or by error)
User forgets about it
User sees that the UI is frozen and gets frustrated, blaming the software.

The safeguards here are:

  • blinking pause button with incrementing update count
  • click on a topology or node un-pauses

We could add more: (1) click on the background un-pauses, (2) show a confirmation dialog.

Could you give this a try @2opremio and see if you get stuck in a paused mode?

User reports bug (or even worse, user does a permanent stop :) )

We certainly dont want to undermine the utility of the product as a whole. If this button is too dangerous, we should simply close this PR.

@davkal
Copy link
Contributor Author

davkal commented Mar 4, 2016

Branch pause-topology-only keeps the topology fixed, but allows clicking/updating nodes.

@pidster
Copy link
Contributor

pidster commented Mar 7, 2016

This is inherently impossible/difficult to manage in the UI because the nodes may be gone by the time you explore them at which point in time a request for details would come back empty.

True until a timestamp parameter is supported & queries can be made against a history of reports...

@foot
Copy link
Contributor

foot commented Mar 7, 2016

I really like the idea of being able to completely pause and inspect the system. Though due to current limitations this would be a tricky do.

  • Grab all topologies data for every topo.
  • Do a details call for every node in every topo.
  • Store all this info.

Maybe for the next one.

Small things:

  • Maybe include the label in the button here.
    screen shot 2016-03-07 at 11 55 21
    • perhaps swapping "paused" for "resume" onMouseOver? Or just the tooltip.
  • Set cursor: pointer; on all the buttons down here.
  • If you have a details panel open and hit pause, the buffered chart animation keeps moving.
  • The "indirect" (changin topo/clicking node) unpausing transition is a bit subtle, I didn't realise that I had unpaused the system by performing those actions.
    • Perhaps leave the "resuming" there for 3-4s, maybe w/ a darker bg/border that fades out.
    • orr set content of new topo/details panel to be a big fat play button.

@@ -357,7 +372,13 @@ export class AppStore extends Store {
this.__emitChange();
break;

case ActionTypes.CLICK_PAUSE_UPDATE:
updatePausedAt = new Date;

This comment was marked as abuse.

@foot
Copy link
Contributor

foot commented Mar 7, 2016

Being able to click around is quite nice!

Pros:

  • Solves a known use case more clearly.

Cons:

  • Some nodes will respond w/ a 404 i guess. (happens w/ both proposed solutions though).
  • Confuses the meaning of paused a little as less stuff in the UI is paused.

Its easier to forget the topo is paused if you're off exploring details, I'd make the pause state a tiny bit more prominent, maybe bold + leave the border around the button + label when activated.

I'm +1 on pause-topology-only though.

* pauses topology and details panel updates
* buffers requests
* un-pause plays back the buffered requests
* automatically un-pauses when topology is changed, or other node is
  clicked (then discards buffer)

Fixes #1025
@davkal davkal force-pushed the 1025-pause-button branch from 2b887a6 to d7507cf Compare March 7, 2016 14:26
@foot
Copy link
Contributor

foot commented Mar 7, 2016

LGTM after TAL! Button looks slick.

Use case coverage:

  • + Can pause and look over the topo and see how transient nodes are connected.
  • - Probably can't see the details of these transient nodes as the BE won't know about them anymore after a second or two.

But I think this is a good start!

davkal added a commit that referenced this pull request Mar 7, 2016
@davkal davkal merged commit cfc3d4f into master Mar 7, 2016
@davkal davkal deleted the 1025-pause-button branch March 7, 2016 16:42
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.

6 participants