Global keyboard shortcuts#29
Conversation
joshuacwnewton
left a comment
There was a problem hiding this comment.
This mostly works! Just a few notes:
- Did we remove the use of
j/kfor scrolling up/down? (I assume we did at some point to allow typingjandksafely in the search bar...)- This isn't really something that needs to be fixed, but I personally have that muscle memory from RSS feed readers, aha. I bet most of our users don't, though.
- Maybe that means we can close spinalcordtoolbox/spinalcordtoolbox#3466
- Currently, the user has to click on a row to begin using keyboard shortcuts (i.e. pressing
downdoesn't automatically highlight the first row). Ideally they would be able to use the keyboard right from the get-go?- Instead of the
down= highlight first row, maybe the first row could be automatically focused on page load? Not sure which is easier...
- Instead of the
- Clicking on the "fit to height / full size" button pair still hijacks the right arrow key. Is there any way to avoid this behavior and make sure the right arrow key always toggles the overlay? e.g. making that button pair two single buttons?
|
Thank you @elhil ! How can we test this PR (given the code is not part of SCT, I'm not sure what's the best way to proceed)? |
The repository contains a sample dataset (no need to run with SCT). There are some instructions for building and opening the sample QC report in the README of this repo: https://github.com/spinalcordtoolbox/report-ui#making-changes |
I don't think this repo ever did that, but I went ahead and added it (very easy with this refactor). As to the search bar, the hotkey library by default doesn't capture keys when certain types of elements are in focus, which is nice (e.g. typing 'd' doesn't toggle image fit, and arrow keys work). I would say spinalcordtoolbox/spinalcordtoolbox#3466 is closeable though, given the behaviour of the keys right now.
This is a regression introduced by this PR. Fixed!
Looks like the hotkey library supports specifying under which types of elements our global hotkeys still apply. So this is done as well :) |
Refactor to allow for global keyboard shortcuts. Use react-hotkeys-hook to allow more consistent handling of keys. Since keyboard shortcuts modify table state, lifts tracking of state up to App component.
fabed11 to
ad9a4c0
Compare
ad9a4c0 to
9ed57bf
Compare
|
Test QC report built! You can download it here. |
joshuacwnewton
left a comment
There was a problem hiding this comment.
I only had one nitpick, but everything else LGTM, so I'm happy to pre-approve since the fix is so simple.
[skip ci]
86322e6 to
a9b7906
Compare
Refactor to allow for global keyboard shortcuts. Use react-hotkeys-hook to allow more consistent handling of keys. Since keyboard shortcuts modify table state, lifts tracking of state up to App component.
Resolves #27 and spinalcordtoolbox#5144