mattermost-webapp
mattermost-webapp copied to clipboard
mattermost
MM-37874: Add zoom + pan function to image preview
Summary
Continuation of the work from https://github.com/mattermost/mattermost-webapp/pull/10923
Adds zoom and pan functionality to image preview along with a zoom control toolbar, similar to the one in PDF.js
Also, the toolbar text (Automatic, Fit width, Fit height) is international but only has English, French, Spanish, Italian and German translations.
Ticket Link
Related issue: https://mattermost.atlassian.net/browse/MM-37874
Related Pull Requests
https://github.com/mattermost/mattermost-webapp/pull/10923
Screenshots
| before | after |
|---|---|
 |
https://user-images.githubusercontent.com/108882245/184133772-2ad24a59-4ef2-49bb-8ca4-30acf7589d12.mp4 |
Release Note
* Added image preview control support
Hello @antonbuks,
Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.
E2E tests not automatically triggered, because PR has no approval yet. Please ask a developer to review and then try again to attach the QA label.
Mattermost test server created! :tada:
Access here: https://mattermost-webapp-pr-11152.test.mattermost.cloud
| Account Type | Username | Password |
|---|---|---|
| Admin | sysadmin | Sys@dmin123 |
| User | user-1 | User-1@123 |
this is looking great .. just a few minor things/comments
WIP, should be wrapped up for tomorrow :)
This PR has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!
New commit detected. SpinWick will upgrade if the updated docker image is available.
Mattermost test server updated with git commit fba28d3722c0c6e3324fa847754399e72e30590d.
Access here: https://mattermost-webapp-pr-11152.test.mattermost.cloud
Successfully triggered e2e testing! https://git.internal.mattermost.com/qa/cypress-ui-automation/-/pipelines/248467
Thanks @antonbuks E2E report shows few failures related to changes. Please take a look at the image preview failures. Noticed few issue when testing manually
-
No scroll bars; vertical and horizontal bars are missing when image is zoomed in
-
Zoom is applied to all images in a group e.g. Attach 2 or more images to the same post, open preview. If you apply zoom to the 1st image at 200% and move on to the next it will also display at 200% zoom. I suggest we only apply the zoom to image being previewed.
-
In mobile webview, header and footer are cut off slightly, also no scroll bars
-
On Windows and Ubuntu, menu didn't display correctly, it appears to be a theming issue, menu items were present on mouse hover
-
I missed having a dedicated reset zoom button. Instead user has to open the menu and click on the "Automatic" to reset. Initially, I thought, that option was something similar to "fit the screen". "Automatic" just wasn't descriptive enough for me @matthewbirtch thoughts?
5. I missed having a dedicated reset zoom button. Instead user has to open the menu and click on the "Automatic" to reset. Initially, I thought, that option was something similar to "fit the screen". "Automatic" just wasn't descriptive enough for me @matthewbirtch thoughts?
This is good feedback @jgilliam17. I don't know if it's a blocker for this PR though. I would say it would be good to add an enhancement once this is merged to offer some sort of 'reset' button. The work done here is based on a Mozilla library which uses the same idea of 'automatic zoom': https://mozilla.github.io/pdf.js/web/viewer.html
@antonbuks what is the difference between Automatic and 100% or Actual size? I assume Automatic means that it will fit to screen unless the image is smaller in which case it will be scaled to it's actual size. For images larger than the viewport, I assume Automatic fits to screen?
This PR has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!
@matthewbirtch tbh I'm not too sure as I didn't do the initial implementation, from the top of my head I think these options were well explained in the figma attached in the jira ticket.
If I had to guess based on tests, for larger images Automatic seems to be fitting width or height based on image dimensions, whichever fits best for the image, and the individual fit options let you choose.
For images smaller than the viewport Automatic seems to conserve image dimensions while the fit options will scale up the image.
@jgilliam17 I think we should merge this as it adds great value to the gallery view as-is. If you want, we can create a follow-up ticket to address the need for a 'reset' button after the fact.
@matthewbirtch thanks - I don't see any of my comments addressed/fix applied yet see comment here, additionally e2e tests are broken due to changes in this PR
- 2 , 3 and 5 can definitely be follow up tickets
- 4 seems significant enough to hold the PR until a fix is provided as the dropdown menus appeared empty, likewise for missing scrollbars 1.
@matthewbirtch thanks - I don't see any of my comments addressed/fix applied yet see comment here, additionally e2e tests are broken due to changes in this PR
- 2 , 3 and 5 can definitely be follow up tickets
- 4 seems significant enough to hold the PR until a fix is provided as the dropdown menus appeared empty, likewise for missing scrollbars 1.
My apologies. I missed those somehow. I think all of your comments should be fixed before merging, with the exception of 5. I think we should defer 5 to a follow-up.
@antonbuks Are you able to look into the issues mentioned here. Let us know if you need help getting this across the finish line. 🙂
@jgilliam17 sorry I somehow missed the notification for this, yes I will try to take a look this week and let you know.
@michelengelen been a little swamped at work lately so just haven't had time to look into it yet unfortunately, if anyone wants to help me get this over the finish line I can grant moderator priviliges to my fork.
Also we have this running on our fork for a while now and we noticed some quality issues with large images using the canvas method so we were experimenting just using an img tag and handling zoom with css and I was hoping to pr this back along with these changes.
@antonbuks Sounds good! If you find a suitable solution let me know ... also: @matthewbirtch or @esethna do we have the capacity and need to work on this ourself (I certainly have not enough time for it), or do we wait for it to be done?
here is the quality issue we noticed, as you can see with the canvas method the preview is very blurry while the original is very high res (3024 × 4032, over 10mb can't upload to github)
