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

New image lightbox should zoom to 100% on image click #17049

Closed
jryans opened this issue Apr 23, 2021 · 2 comments · Fixed by matrix-org/matrix-react-sdk#5916
Closed

New image lightbox should zoom to 100% on image click #17049

jryans opened this issue Apr 23, 2021 · 2 comments · Fixed by matrix-org/matrix-react-sdk#5916

Comments

@jryans
Copy link
Collaborator

jryans commented Apr 23, 2021

At the moment, the new image lightbox seems to zoom to an arbitrary 300% when clicking the image, which is somewhat surprising compared to other image views, such as browsers when they display an image file.

I think it would be more natural to match the behaviour of other image viewers:

  • For small images that fit in the viewport, display them at 100% by default, and clicking the image is disabled
  • For large images that would overflow the viewport, shrink to fit by default, and enable clicking the image to toggle between 100% and shrink to fit
@jryans jryans added A-Media Help Wanted Extra attention is needed T-Enhancement labels Apr 23, 2021
@SimonBrandner
Copy link
Contributor

SimonBrandner commented Apr 23, 2021

@niquewoodhouse, do you have any thoughts on this? I think I might be able to do something like this but I am not 100% sure (css can be weird :D)

Edit: It's not that much about if it is possible but it's more about the fact that it can get a bit complex

@SimonBrandner
Copy link
Contributor

SimonBrandner commented Apr 24, 2021

I've created a draft PR. It seems to work great, there is just one thing missing - the max and min zoom is calculated based on the size info from the event but it seems that this data can be missing. I am not really sure what to do in that case

Edit: I guess we can always fall back to the current behaviour

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

Successfully merging a pull request may close this issue.

2 participants