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

Add keyboard shortcut to start/stop drawing annotation #154

Merged
merged 4 commits into from
May 16, 2022

Conversation

mvandenburgh
Copy link
Member

Pressing n will

  • Open the New Annotation dialog if no annotations are selected
  • Put the user into or cancel "draw mode" for the current annotation if one is selected

Fixes in part #145

@mvandenburgh mvandenburgh requested a review from manthey May 11, 2022 17:37
@mvandenburgh
Copy link
Member Author

@manthey coverage is failing - do you usually have tests for things like keyboard shortcuts?

@manthey
Copy link
Contributor

manthey commented May 11, 2022

This starts drawing a line. Were you thinking of adding additional keyboard bindings for the other shapes? Or should we have a concept of a recent shape and choose that? And, if so, then we might want another key to cycle through the shapes.

@manthey
Copy link
Contributor

manthey commented May 11, 2022

@mvandenburgh
Copy link
Member Author

mvandenburgh commented May 11, 2022

This starts drawing a line. Were you thinking of adding additional keyboard bindings for the other shapes? Or should we have a concept of a recent shape and choose that? And, if so, then we might want another key to cycle through the shapes.

Additional keyboard bindings for each shape would be easiest to implement code-wise and UX-wise, I think. (just adding keyboard shortcuts to each of the shapes' tooltips seems easier than trying to communicate what the recent shape is to the user). I'll try that.

We do test some keyboard shortcuts (e.g., https://github.com/DigitalSlideArchive/HistomicsUI/blob/master/tests/web_client_specs/annotationSpec.js#L327).

Thanks, I'll take a look at those and write some tests for the new shortcuts.

@mvandenburgh mvandenburgh marked this pull request as draft May 11, 2022 19:13
@manthey
Copy link
Contributor

manthey commented May 11, 2022

I think using letters that are somehow associated with the shape will be more memorable than using the top right six letters (but I'm willing to be argued out of it). Maybe:
o point
r rectangle
l ellipse
c circle
p polygon
n line

@mvandenburgh
Copy link
Member Author

I think using letters that are somehow associated with the shape will be more memorable than using the top right six letters (but I'm willing to be argued out of it). Maybe: o point r rectangle l ellipse c circle p polygon n line

I don't have a strong preference, but I can see it being easier to remember if they are associated with a letter. I'll change it to those

@mvandenburgh mvandenburgh force-pushed the draw-annotation-keyboard-shortcut branch 2 times, most recently from 894c331 to e88dec8 Compare May 12, 2022 22:45
@mvandenburgh
Copy link
Member Author

I added tests for the new shortcuts. Looks like CI is happy with coverage now.

@mvandenburgh mvandenburgh marked this pull request as ready for review May 12, 2022 22:50
@mvandenburgh mvandenburgh force-pushed the draw-annotation-keyboard-shortcut branch from e88dec8 to 5277693 Compare May 16, 2022 14:10
@mvandenburgh mvandenburgh force-pushed the draw-annotation-keyboard-shortcut branch from 5277693 to cd57963 Compare May 16, 2022 14:12
@mvandenburgh mvandenburgh force-pushed the draw-annotation-keyboard-shortcut branch from 48e72b4 to 4aa765d Compare May 16, 2022 14:24
@mvandenburgh mvandenburgh requested a review from manthey May 16, 2022 14:30
@mvandenburgh mvandenburgh requested a review from manthey May 16, 2022 15:50
@mvandenburgh mvandenburgh merged commit 97d6d1e into master May 16, 2022
@mvandenburgh mvandenburgh deleted the draw-annotation-keyboard-shortcut branch May 16, 2022 15:56
@mvandenburgh mvandenburgh mentioned this pull request May 16, 2022
4 tasks
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.

2 participants