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

Touch to zoom img preview #7734

Closed
wants to merge 2 commits into from
Closed

Touch to zoom img preview #7734

wants to merge 2 commits into from

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented Jan 8, 2018

  • Request a preview that covers the area and not fills it (better to have a few more pixels)
  • Remove link of the preview so that tapping it works once again

Ask for a preview to cover the area. This makes sure we get a preview of
at least the requested size. But it is better to have the browser scale
down than up.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
This should enable tap to zoom once again

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@codecov
Copy link

codecov bot commented Jan 8, 2018

Codecov Report

Merging #7734 into master will increase coverage by 0.05%.
The diff coverage is 71.42%.

@@             Coverage Diff              @@
##             master    #7734      +/-   ##
============================================
+ Coverage     51.12%   51.18%   +0.05%     
  Complexity    24948    24948              
============================================
  Files          1605     1605              
  Lines         94923    94922       -1     
  Branches       1376     1376              
============================================
+ Hits          48528    48583      +55     
+ Misses        46395    46339      -56
Impacted Files Coverage Δ Complexity Δ
...sharing/lib/Controller/PublicPreviewController.php 36.36% <100%> (ø) 18 <0> (ø) ⬇️
apps/files_sharing/js/public.js 48.74% <66.66%> (-0.26%) 0 <0> (ø)
lib/private/Server.php 81.43% <0%> (-0.12%) 134% <0%> (ø)
lib/private/Security/CertificateManager.php 92.07% <0%> (+0.99%) 39% <0%> (ø) ⬇️
lib/private/Files/Cache/Propagator.php 96.2% <0%> (+1.26%) 16% <0%> (ø) ⬇️
lib/private/Memcache/Memcached.php 57.89% <0%> (+57.89%) 31% <0%> (ø) ⬇️

@jancborchardt
Copy link
Member

jancborchardt commented Jan 10, 2018

Hm, clicking an image doesn’t really seem to do anything? (Previously it opened the /preview URL) How to test this? :)

@rullzer
Copy link
Member Author

rullzer commented Jan 10, 2018

@jancborchardt well now I'm confused... #7614 (comment)

if you want to keep opening the /preview then we can just close this PR... However, we can't make it open the full original image as I mentioned. So your call.

@rullzer
Copy link
Member Author

rullzer commented Jan 10, 2018

Mmmm ok so after thinking I think the opening of the preview just makes more sense. And now that we have higher default sizes (only for newly uploaded pictures). Opening /preview should be enough for now.

Lets close this and if we want to have it do more/different think about this for 14.

@rullzer rullzer closed this Jan 10, 2018
@rullzer rullzer deleted the touch_to_zoom_img_preview branch January 10, 2018 12:22
@jancborchardt
Copy link
Member

Ok, cool. I guess we were both confused there. :)

And yeah, your other fix already does a lot of good towards not always needing to get a larger image.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants