nextcloud
Fix scroll to zoom, resize small images to fill
Setting the background to white is removed since it doesn't work well with the zooming (Outside the image boundaries is still transparent).
Transitions are disabled when zooming (or rather only enabled when zooming back to 1x or in to 1.3x) because they don't work well with zooming (feels laggy).
@skjnldsv Can you please take a look at my replies (in particular the one about the hover background)
Also I'm assuming the Node / node test failed because I didn't commit the correct js/viewer-main.js and js/viewer-main.js.map files?
Also I'm assuming the
Node / nodetest failed because I didn't commit the correctjs/viewer-main.jsandjs/viewer-main.js.mapfiles?
Yes :) You can either do it here or ask the bot to do it for you with one of the following comments:
/compile /jswill create a new commit containing the compiled files/compile amend /js, will amend your previous commit/compile fixup /jswill create a fixup based on the previous commit
Which issue does this PR close? Please link it. Thanks! :)
I don't think there are any
I don't think there are any
Thanks for the answer! So it doesn't fix https://github.com/nextcloud/viewer/issues/916?
Thanks for the answer! So it doesn't fix #916?
No, the mobile drag/zoom APIs are quite different.
No, the mobile drag/zoom APIs are quite different.
Thanks for letting me know. The PRs title sounded like it coulld have fixed the mentioned issue.
The PRs title sounded like it could have fixed the mentioned issue.
What the scroll to zoom meant is that previously, whenever I scroll on an image it just moves in seemingly random directions (the zoom point doesn't stay under the cursor).
@skjnldsv I think I've done all the changes you requested in https://github.com/nextcloud/viewer/pull/1063/commits/aa829e43a0ed62d69a0df95799b7ca53165d2c87
@skjnldsv https://github.com/nextcloud/viewer/pull/1063/commits/70957b6cbbea5e3c524a997e3bbcbb9f5c7991e7 is because I set up YouCompleteMe, so I also fixed a few vls diagnostic warnings (all in the doc comments). If you don't want the jsconfig.json file or these warning fixes, I'll revert the commit.
Edit: https://github.com/nextcloud/viewer/pull/1063/commits/940711625a8c8f5f56cc5402e12021b5499aca03 (turns out I didn't have let g:ycm_max_diagnostics_to_display = 0 in my vimrc, so I wasn't getting all the diagnostic warnings)
Should I squash the handling of transparent images together with the fix zoom commit?
Also, why are the built js files stored in the repo? It makes it so much harder to make git changes like fixups (that I then rebase -i) because I need to also deal with the files in js/.
@skjnldsv The way to fix both the title and arrows being over the image is to put the image in a smaller box that's not overlapping with the title and arrows.
The reason why small images need to be enlarged is because the viewer app shows the thumbnails generated using previewgenerator, and to save space people might store their thumbnails at a resolution lower than their browser window.
The reason why small images need to be enlarged is because the viewer app shows the thumbnails generated using
previewgenerator, and to save space people might store their thumbnails at a resolution lower than their browser window.
That's a very specific but interesting use-case. Might be worth showing a warning to the admin if this is the case. :thinking:
Also should I do the fix I suggested and put the image in a smaller box?
@skjnldsv Sorry, I don't get why a warning would need to be shown?
On the overview page, we should warn admins that set a max_preview config to low, that it will impact their experience negatively. This is not for this PR
Also should I do the fix I suggested and put the image in a smaller box?
Not my call, up to the UI/UX team to approve
that it will impact their experience negatively.
Especially with this PR, I don't see how it will cause a negative impact on experience?
I don't see how it will cause a negative impact on experience?
- have a 20MP image
- Open it
- See pixelated zoomed image