1372387 - Re-trigger build needs feedback on click

Status: RESOLVED FIXED

(Tree Management :: Treeherder, enhancement)

Description

[Erica Wright [:ewright]]

I'm on try/using Treeherder for the first time, and attempted to re-trigger a build. Because of a lack of feedback from clicking the re-trigger button, I clicked it multiple times, believing I had missed it.

I believe this sort of thing is likely to clog up the queue/use unnecessary resources.

We could use a better UX here to reduce this sort of mistake.

Comment 1

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

The feedback should be the "retrigger request sent" notification up in the corner of the screen, but maybe we should add something to the button itself.

Comment 2

[William Lachance (:wlach)]

(In reply to Erica from comment #0)
> I'm on try/using Treeherder for the first time, and attempted to re-trigger
> a build. Because of a lack of feedback from clicking the re-trigger button,
> I clicked it multiple times, believing I had missed it.
> 
> I believe this sort of thing is likely to clog up the queue/use unnecessary
> resources.
> 
> We could use a better UX here to reduce this sort of mistake.

I investigated this a bit and we do in fact show a small notification when the job is clicked on the top of the screen.

However, it is true that the button itself does not show any feedback that it was interacted with, which is a bit confusing and might contribute to an impression that nothing happened if you're not watching the whole screen. :) I think we can fix this fairly easily by adding some CSS for the "active" state of the buttons and links in the info bar.

Comment 3

[William Lachance (:wlach)]

I'll take this.

Comment 4

[GitHub Autolander Bot]

Attachment: [treeherder] wlach:1372387 > mozilla:master

Comment 5

[Jonathan French (:jfrench)]

Comment on attachment 8876912 [details] [review]
[treeherder] wlach:1372387 > mozilla:master

I left a comment in the PR with another possible approach, in case it's helpful :) I defer to you guys as to what makes most sense.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: PR 2555

Here's one based off of the pulsing that the pinboard count icon does when you pin a job. At the moment it only does it if you click the retrigger button, but that could easily be changed so it also pulses the button if you it 'r'.

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 8876912 [details] [review]
[treeherder] wlach:1372387 > mozilla:master

I like both approaches, to be honest.

Comment 8

[Jonathan French (:jfrench)]

Ya, my only logic for a single css rotation on that circular icon is it would mimic common UI out there telling you 'something is happening' and so the button wouldn't grow in size (unlike our pinboard counter). For example, the rotating icon when you reload a Firefox tab, Chrome tab, et cetera.

Comment 9

[William Lachance (:wlach)]

How about we do both? My patch to implement the expected default behaviour, KWierso's for frosting on top. :)

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

It does look pretty nice with the rotate. I've updated my PR to do it. Video capture of what it does: https://www.youtube.com/watch?v=al65r_5qQKs

(Ignore the 'disabled'ness of the hover icon and the notification in the corner. I commented out the ng-disabled bit on the button itself so clicking it does stuff, but the rest of the code still rejects unauthenticated retrigger attempts.)

Comment 11

[Jonathan French (:jfrench)]

Ya, that looks pretty cool! Maybe there's a way to slow down the ease-out an extra 0.2s or something at the end (so it slows to a stop), but I like it.

Comment 12

[Jonathan French (:jfrench)]

(In reply to William Lachance (:wlach) (use needinfo!) from comment #9)
> How about we do both? My patch to implement the expected default behaviour,
> KWierso's for frosting on top. :)

That sounds good, since that will provide active emphasis for any button not just retrigger? I think the chosen css color was fine, or near-black would be fine too, imo.

Comment 13

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/cbb8d25e158831c25c1a2145a6ba2288fde79c61
Bug 1372387 - Fix CSS override of button state in infobar panel

This ensures we show a "shadow" when the retrigger/option buttons
are hovered over

https://github.com/mozilla/treeherder/commit/d4b5c66739762d5f66fd1964308af7da358935aa
Bug 1372387 - Add an active state to the infobar buttons

This makes it more visible when they are clicked

Comment 14

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/1323ba755e70898d046d41d4b2bef01e68daf5c6
Bug 1372387 - Spin the retrigger button when clicked (#2555)

* Bug 1372387 - Spin the retrigger button when clicked

Comment 15

[GitHub Autolander Bot]

Attachment: [treeherder] KWierso:fixspinningbutton > mozilla:master

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 8882096 [details] [review]
[treeherder] KWierso:fixspinningbutton > mozilla:master

So I landed it tonight and noticed an issue with it. If you retrigger a job with the 'r' keyboard shortcut, it spins just the retrigger button's icon, like you'd expect. If you click the button, though, it spins the entire rectangular retrigger button.

This PR should fix it. I have no idea why the two retriggering methods have different behaviors, since they call the same function, but this PR should get things working by targeting the icon within the button specifically.

Comment 17

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/8a20c8cbdeccb98900ec333f47fc935c654e7c64
Bug 1372387 - Followup to only spin the retrigger button's icon (#2599)

Comment 18

[William Lachance (:wlach)]

Comment on attachment 8882096 [details] [review]
[treeherder] KWierso:fixspinningbutton > mozilla:master

lgtm, went ahead and merged