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

#2347 - Build Image viewer in the app #2367

Open
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

SebinSong
Copy link
Collaborator

closes #2347

[ Screenshot - Desktop ]



[ Screenshot - Mobile ]

On mobile browser, zooming in/out can be done both by using the slider at the bottom and via pinch zoom gestures. (tested on my Android and worked well)



This viewer is triggered by clicking on two places in the chatroom.

@SebinSong SebinSong self-assigned this Oct 2, 2024
@SebinSong SebinSong marked this pull request as draft October 2, 2024 19:31
@SebinSong SebinSong changed the title [WIP] #2347 - Build Image viewer in the app #2347 - Build Image viewer in the app Oct 2, 2024
@SebinSong SebinSong marked this pull request as ready for review October 2, 2024 19:55
Copy link

cypress bot commented Oct 2, 2024

group-income    Run #3240

Run Properties:  status check passed Passed #3240  •  git commit bac43600b0 ℹ️: Merge 48db233624eb579ebb9b4f10f8fecb74e0f84366 into b64ecda81d1b1c9574b1b4490945...
Project group-income
Run status status check passed Passed #3240
Run duration 09m 24s
Commit git commit bac43600b0 ℹ️: Merge 48db233624eb579ebb9b4f10f8fecb74e0f84366 into b64ecda81d1b1c9574b1b4490945...
Committer Sebin Song
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 10
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 111


export default {
// NOTE: gave this component a generic name in case this is used outside the chatroom area. (eg. instead of 'ChatImageViewer' etc.)
name: 'ImageViewerModal',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the filename has a typo in it :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch. fixed.

Copy link
Member

@taoeffect taoeffect left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic work @SebinSong!!

I tested it and it seems to work almost perfectly!

I found a few issues:

  1. On Firefox Desktop when dragging an image:
bug.compressed.mov

As you can see, sometimes when you drag it too far it will get "stuck" and from that point on you won't be able to keep dragging it.

  1. On mobile, pinching in and zooming is a bit "stuttery" and not smooth.
  2. On mobile, pinching in and zooming does not zoom into the origin point of where you are pinching in to. Instead it zooms from the center. Try uploading a large picture and zooming in - it will zoom using the origin (the center of the image) as the zoom point. This makes it difficult to zoom in on say the top-right of an image, because you have to zoom, and then scroll. You can't just zoom to the point you want to immediately.

EDIT: also a question: how difficult would it be to have the viewer cycle between photos using the ◀️▶️ arrow keys on desktop if the message contains multiple photo attachments? Is this something that you think can easily be done in this PR or should it be saved for a separate issue?

@SebinSong
Copy link
Collaborator Author

@taoeffect

how difficult would it be to have the viewer cycle between photos using the ◀️▶️ arrow keys on desktop if the message contains multiple photo attachments? Is this something that you think can easily be done in this PR or should it be saved for a separate issue?

I think it's something achievable for sure but would love to work on it as a separate PR.

.c-image-view-area(
@wheel.prevent.stop='wheelEventHandler'
)
img.c-preview-image(ref='previewImg'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For accessibility, img elements need an alt attribute

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch

@SebinSong
Copy link
Collaborator Author

@taoeffect

sometimes when you drag it too far it will get "stuck" and from that point on you won't be able to keep dragging it.

How far it can be draggable is calculated by below block,

    movableDistances () {
      // calculates how much vertical/horizontal spaces are available to move image around in.

      if (!this.isImageMovable) {
        return { x: 0, y: 0 }
      } else {
        const percentDiff = (this.ephemeral.currentZoom - this.config.zoomMin) / 100
        return {
          x: this.config.imgData.naturalWidth * percentDiff / 2,
          y: this.config.imgData.naturalHeight * percentDiff / 2
        }
      }
    }

and this is actually based on my observation in the Slack image-viewer where as long as the edge of the image hits the viewer's edge, it apparently decides that those corner/edge areas are visible and stops to move.
For example, in the screenshot below, I can't move further down beyond the below point.

image

@SebinSong SebinSong marked this pull request as draft October 7, 2024 04:50
@SebinSong SebinSong marked this pull request as ready for review October 12, 2024 03:12
@SebinSong
Copy link
Collaborator Author

@taoeffect The PR has been updated with the work for your feedback.

@SebinSong
Copy link
Collaborator Author

@taoeffect
regarding

On mobile, pinching in and zooming is a bit "stuttery" and not smooth.

I decreased the throttling value for pointermove event handler for this which I felt at least improved how smoothly it reacts to my touch actions. Hope this improvement happens to your devices too.

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.

Picture viewer
3 participants