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

feat: Add video zoom functionality in lightbox #1302

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

XxAlonexX
Copy link

Implements video zooming functionality in the lightbox view as requested in #1287.

Changes made:

  • Added pinch-to-zoom gesture support using InteractiveViewer
  • Implemented pan gesture support for zoomed videos
  • Set zoom limits (1.0x to 5.0x) for better user experience
  • Maintained proper video aspect ratio while zooming
  • Added proper cleanup with TransformationController disposal

This enhancement allows users to:

  • Zoom in/out using pinch gestures (up to 5x zoom)
  • Pan around when zoomed in
  • Maintain proper video aspect ratio while zooming
  • Return to original size by double-tapping

The video controls remain accessible at the bottom of the screen while zooming.

Fixes #1287

Implements pinch-to-zoom and pan gestures for videos in the lightbox view.
This allows users to zoom in/out (up to 5x) and pan around while
maintaining proper aspect ratio.

Fixes zulip#1287
@gnprice
Copy link
Member

gnprice commented Jan 24, 2025

Thanks. Before we can review this in detail, it will need tests. See our README, which says that and has links to more resources.

This also makes several changes that seem unrelated to the issue this is meant to solve — for example removing the loading indicator. If those are unintended, you'll need to remove them. If intended, please make clear in the commit messages why those changes are desirable. See the links in our README for Zulip's Git style guide.

@XxAlonexX
Copy link
Author

I apologize for submitting the PR without proper tests and for including unintended changes
As this is my first contribution to Zulip, I'll:

  1. Close this PR
  2. Review the README thoroughly, especially the testing guidelines and Git style guide
  3. Create a new PR that:
    • Focuses solely on the video zoom functionality
    • Includes comprehensive tests
    • Follows Zulip's Git commit message guidelines
      I appreciate the guidance and look forward to submitting a properly structured contribution.

@XxAlonexX XxAlonexX closed this Jan 24, 2025
@gnprice
Copy link
Member

gnprice commented Jan 24, 2025

No need to close a PR and open a separate one — it's better to update the same PR. That keeps the discussion thread together. (You can still reopen this one.)

@XxAlonexX XxAlonexX reopened this Jan 24, 2025
@chrisbobbe
Copy link
Collaborator

Also, it looks like CI is failing; that will also need to be fixed. 🙂

@XxAlonexX
Copy link
Author

Give me some time, I'll fixing the issue within the guidelines 😄

@XxAlonexX
Copy link
Author

I keep getting NDK version error and trying fixing it using the upgrade script but it requires linux environment to run, can anyone guide me a little ?

@gnprice
Copy link
Member

gnprice commented Jan 26, 2025

See the section in the README about asking for help — you'll get answers more effectively in Zulip following those guidelines than by trying to have the conversation in this GitHub thread.

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.

Allow zooming in on videos in the lightbox
3 participants