-
Notifications
You must be signed in to change notification settings - Fork 35
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 support #80
Add keyboard support #80
Conversation
Intial stab at this, I'm not sure all the code lives in the right place and looking for feedback on that. Additional issues/questions: - Is there a way to get the 'standard' system keyboard shortcuts for this? - How to detech ctrl/meta + key press? I never saw them detected though I'm testing with the android emulator which does werid things with external keybaords. - keyboard input enabled by default? note: view needs to be focused to start receiving events - configure zoom & pan steps? Fixes saket#78
key = screen | ||
) | ||
} | ||
val record = backstack.first() |
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.
There's currently a bug where you can still keyboard focus the gallery screen when the media viewer is brought up, I'm not familiar enough with circuit to know what the right fix was so just changed it to render only the current page while testing. That does break the swipe away animation since it won't render the behind page until the animation completes
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.
There's currently a bug where you can still keyboard focus the gallery screen when the media viewer is brought up
hmmmmmm I wonder if I can fix this by preventing touch/focus events from leaking behind the active screen.
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.
On my side project where I'm using a custom navigation solution I solved this by not rendering the behind page unless you were in the middle of the transition.
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.
This is fantastic, thank you!
Are you blocked on this by any chance? I'd like to complete #4 before I merge this, but I can try finding a stop-gap if you need this in your project.
key = screen | ||
) | ||
} | ||
val record = backstack.first() |
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.
There's currently a bug where you can still keyboard focus the gallery screen when the media viewer is brought up
hmmmmmm I wonder if I can fix this by preventing touch/focus events from leaking behind the active screen.
Nope! We ended up adding separate zoom/pan support on top for keyboard support for now. It does mean you could technically do both independently but it seems to work ok enough. |
Took me some time, but I'm finally working on this. I'm going ahead and merging this as-is because your code looks great. I'll make a few adjustments and then land the changes on |
} | ||
return true | ||
} | ||
event.key == Key.Zero -> { |
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'm curious, what was the reason for supporting Key.Zero
? I haven't been able to find any other app/OS that resets zoom when the zero
key is pressed.
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.
Because my browser does, didn't think too much about it and don't have strong opinions
Intial stab at this, I'm not sure all the code lives in the right place and looking for feedback on that.
Additional issues/questions:
Fixes #78