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

web: add pause button to top bar of pipeline view #3475

Merged
merged 7 commits into from Mar 14, 2019

Conversation

@robwhitby
Copy link
Contributor

robwhitby commented Mar 11, 2019

Fixes #2847

Hey, this adds a play/pause button to the top bar so you don't have to go back to the dashboard to update. I don't know Elm very well so please let me know if I've done something weird!

Thanks
Rob

robwhitby added 2 commits Mar 11, 2019
…ed to the Dashboard model

Signed-off-by: Rob Whitby <rob.whitby@springer.com>
Signed-off-by: Rob Whitby <rob.whitby@springer.com>
Signed-off-by: Rob Whitby <rob.whitby@springer.com>
@robwhitby robwhitby changed the title Pause pipeline button topbar web: add pause button to top bar of pipeline view Mar 11, 2019
@pivotal-jamie-klassen

This comment has been minimized.

Copy link
Contributor

pivotal-jamie-klassen commented Mar 12, 2019

This is really great! It's very encouraging to see some really effective reuse of some of the abstractions we've been defining in this project (Effects, inline styles).

There's a couple of UI polish things to help with the user experience:

  • The divider should be #3d3c3c when the pipeline is not paused, and rgba(255, 255, 255, 0.5) (which it currently is) when unpaused.
  • Hover states: The icon itself should have 50% opacity when unhovered and 100% opacity when hovered in both the paused and unpaused cases. This is true except if the user is not authorized to pause/unpause the pipeline (i.e. they have the viewer role on the team to which the pipeline belongs). In that case, the icon should always have 50% opacity and should have a default cursor rather than a pointer cursor.
  • Click behaviour: If the user is authorized to pause/unpause the pipeline, clicking the icon should turn it into a spinner of the same diameter while the API call is in progress, and the page state should update immediately upon the API call resolving (HTTP errors, at this point, generally do not get reported to the user). If the user is unauthorized (like the viewer case described above), clicking should do nothing. If the user is unauthenticated, clicking the icon should redirect to login.

Whatever of those you have the bandwidth and interest to do is awesome. We'll leave this open until we hear further from you, but either way this is much appreciated.

There's an example for the paused divider color at https://github.com/concourse/concourse/blob/master/web/elm/src/TopBar/Styles.elm#L260-L266 with tests at https://github.com/concourse/concourse/blob/master/web/elm/tests/TopBarTests.elm#L216-L221 and https://github.com/concourse/concourse/blob/master/web/elm/tests/TopBarTests.elm#L296-L301

There's an example of the hover and unauthorized/unauthenticated behaviour at https://github.com/concourse/concourse/blob/master/web/elm/src/Resource/Resource.elm#L960-L1006 (the resource check button) with tests at https://github.com/concourse/concourse/blob/master/web/elm/tests/ResourceTests.elm#L2631-L2708 and https://github.com/concourse/concourse/blob/master/web/elm/tests/ResourceTests.elm#L2719-L2811 and https://github.com/concourse/concourse/blob/master/web/elm/tests/ResourceTests.elm#L3062-L3136.

Finally, there's an example of changing to a spinner and back at https://github.com/concourse/concourse/blob/master/web/elm/src/Resource/Resource.elm#L1351-L1400 (the resource pin button) with tests at https://github.com/concourse/concourse/blob/master/web/elm/tests/ResourceTests.elm#L2407-L2427.

@robwhitby

This comment has been minimized.

Copy link
Contributor Author

robwhitby commented Mar 12, 2019

great, thanks for the quick feedback. I will have a look in the next few days.

robwhitby added 4 commits Mar 12, 2019
Signed-off-by: Rob Whitby <rob.whitby@springer.com>
Signed-off-by: Rob Whitby <rob.whitby@springer.com>
Copy link
Contributor

pivotal-jamie-klassen left a comment

This is great. Unless you're planning on pushing more commits, we can definitely add the other tests/behaviour in a subsequent PR. Thanks!

@robwhitby

This comment has been minimized.

Copy link
Contributor Author

robwhitby commented Mar 13, 2019

yep agree with idea of splitting the rest into another PR. And thanks again for such detailed feedback, the project is quite daunting at first but I am really getting to like elm :)

@vito
vito approved these changes Mar 14, 2019
Copy link
Member

vito left a comment

Ooh, this is neat! At first I was worried that this added it to the breadcrumb, so I was concerned about potential mis-clicks, but it's actually off to the right by the pin icon, and only shows when viewing the pipeline itself, so 👍

For anyone curious, here's how it looks:

screencapture-localhost-8080-teams-main-pipelines-booklit-2019-03-14-12_29_47

Echoing @pivotal-jamie-klassen's statement it's really nice to see how far we've come in the web UI. Really encouraging seeing external folks able to hop in and add substantial features like this without too much trouble. Nice job everyone!

@vito vito merged commit 700fc71 into concourse:master Mar 14, 2019
5 checks passed
5 checks passed
DCO DCO
Details
WIP Ready for review
Details
concourse-ci/testflight Concourse CI build success
Details
concourse-ci/unit Concourse CI build success
Details
concourse-ci/watsjs Concourse CI build success
Details
@robwhitby robwhitby deleted the robwhitby:pause-pipeline-button-topbar branch Mar 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.