-
-
Notifications
You must be signed in to change notification settings - Fork 595
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
keyboard handlers patch #1414
keyboard handlers patch #1414
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Just one comment.
Also, please add doc comments in for the event... You can look at other raiseEvent
calls in the file to see examples. Here's what they end up looking like once they are processed:
http://openseadragon.github.io/docs/OpenSeadragon.Viewer.html#.event:canvas-click
src/viewer.js
Outdated
preventHorizontalPan: event.preventHorizontalPan | ||
}; | ||
|
||
this.raiseEvent( 'canvas-key-down', canvasKeyDownEventArgs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that we're using just one event name instead of differentiating between key down and key press (which seems like it would be unnecessary in our context). I'm a little concerned that calling it canvas-key-down
implies that it's specifically a key down event. Maybe we should call it just canvas-key
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I used the same name so it's more "transparent" for the user, that does not need to know about the difference in the backend.
Sure, I agree about changing the event name. ;)
Now that I think of it, I think we only need one copy of the doc comment, even though it's raised in two places. I'll delete one after I merge this. Thank you for making this patch; it came together nicely! |
Fixes #1405.
Keyboard handlers patch:
Example of usage:
Built and tested with grunt: