Swipe Down support on Media files #5622
Conversation
- Added FullScreenGestureListener for handling swipe logic Signed-off-by: rapterjet2004 <[email protected]>
|
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/5622.apk |
AndyScherzinger
left a comment
There was a problem hiding this comment.
@rapterjet2004 The swipe itself works fine but the closing animation is a slide l-t-r instead of a top to bottom. The animation makes sense for the classic back arrow of course, yet if possible for the swipe down it should match the gesture itself. Also fine to have it as a follow up, this is the blocking a merged from my pov
|
After thinking about it, I think it might be better to refactor these full screen viewers to use fragments instead in a follow up PR. That way they can just use a Bottom Sheet, which IMO has a better UX. @AndyScherzinger thoughts? |
|
Sounds good to me for a follow up but don't think that would be a priority right now. For example getting the sdk bumped to 36 to unblock library updated from being merged would be more important. I am also fine with the current state since it is already an improvement but leave the review to @mahibi |
There was a problem hiding this comment.
This PR breaks zooming and the toggle to show/hide appbar/navbar.
Yes it would be much nicer with an animation.
Also, both swiping up and down should close it?
Regarding the refactoring i suggest to take a step back and take into account more requirements,
e.g.:
But i agree it's not a high priority and other things are more important.
🏁 Checklist
/backport to stable-xx.x