Closed Bug 1033268 Opened 12 years ago Closed 12 years ago

Keyboard shortcut for adding selected job to pin board is different from TBPL

Categories

(Tree Management :: Treeherder, defect, P2)

Product:
Tree Management
Component:
Treeherder
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: emorley, Assigned: jfrench)

References

(
URL
)

Details

I think a complete re-work of keyboard shortcuts isn't a bad idea in the future, but to ease the transition to TBPL I think we should aim to make things "just work" using the old shortcuts :-) In treeherder, to select multiple jobs, they have to be added to the pin board. The shortcut for this is 's' vs TBPL's spacebar.
I'll have a look at this one.
Assignee: nobody → tojonmz
URL: https://treeherder.mozilla.org
Status: NEW → ASSIGNED
This introduces some interesting changes in UI behavior. I want to describe them here so everyone is in agreement. Sorry it's a bit long. Using spacebar for a pin event, the spacebar will have to be suppressed in the main job page as a browser 'page down' event. Otherwise every time you add a job to the pin board the job page will also scroll. I have already fixed this part locally. I gather the user will instead be expected to use page down or page up keys, or arrow keys, or scroll bar, to navigate the jobs page while the job-detail/pin board is open. Since spacebar will no longer do that. I had considered trying to 'test' whether the selected job is already in the pin board and if so prevent a pin event, which I think would allow the spacebar keyboard event to 'flow over' into the browser. But that 'flow over' would only be triggered if the job was present in the pin board. If it was not, any other job selected on the jobs page, spacebar will pin it (which is the gist of the request anyway). It seemed odd that TBPL uses a keyboard shortcut that overrides navigation behavior. If you are in agreement you want that, I will do a bit more testing of my change and then submit a PR. Just to spark other ideas, if TBPL transition to TH is not a long term issue, you could alternately consider something different yet again, that is cued off its actual name. eg. 'p' for pinning. But I see 'p'(and k) and 'n'(and j) in the code are being used respectively to navigate back to the "previous" and "next" job failures. So you'd want to consider maybe addressing them as a whole. But I have that change for spacebar, whatever you guys collectively prefer.
Flags: needinfo?(emorley)
I'm adding RyanVm to needinfo, since edmorley is on pto this week
Flags: needinfo?(emorley) → needinfo?(ryanvm)
Answered on IRC - I think we should keep the spacebar functionality as close to what TBPL has as possible.
Flags: needinfo?(ryanvm)
Ok, will do. I have one more thing to sort with the change to spacebar, then I will put up the PR.
The PR looks correct on all servers (dev/stage/prod) so marking this bug Fixed.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
It turns out we need to provide Mac support also - with Cmd+click since that was supported in TBPL on OSX. Ctrl+click on Mac just fires a context menu and the log which is not what we want. Cameron will be looking at that, since I'm windows only here.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://github.com/mozilla/treeherder-ui/commit/7e557a3ebaa2c0ae41498b0485b9c87645f33c4b Added the mac support in this commit.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
I ended up switching status on the wrong bug here. Comment#8 and Comment#9 was intended for bug 1033266. I will propagate them there so we have correct book keeping.
You need to log in before you can comment on or make changes to this bug.