Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Dynamic max and min zoom in the new ImageView #5916

Merged
merged 20 commits into from
Apr 26, 2021
Merged

Dynamic max and min zoom in the new ImageView #5916

merged 20 commits into from
Apr 26, 2021

Conversation

SimonBrandner
Copy link
Contributor

@SimonBrandner SimonBrandner commented Apr 24, 2021 •
edited
Loading

Fixes #17049 and does a bit of refactoring

@SimonBrandner
Initial code for dynamic minZoom
dad7a22 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Add dynamic maxZoom and wire it all up
4523b8c 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Don't use percanteages
f8af983 
I was an idiot to use them in the first place

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Get rid of onWindowResize()
57b34f8 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Remove logline
e0e9ccb 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Use correct cursor when we can't zoom
dcc060f 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Merge branch 'develop' into image-view-zoom
1a906c9
@SimonBrandner
Use MAX_SCALE for maxZoom
9b7a9fc 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Add some comments
bcc6e5c 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Fix zoom step and coeficient
90f2423 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Rework zooming
0e31297 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Use a ref instead of that ugly thing
c1a4204 
Sometimes I do really weird things and don't know why :D

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@jryans
Copy link
Collaborator

jryans commented Apr 26, 2021

Have you tested this with images smaller than the viewport? At the moment, this PR seems to still scale them, which I don't think we want.

@SimonBrandner
Fall back to natural height and width
33107a1 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@jryans
Copy link
Collaborator

jryans commented Apr 26, 2021

Here's an example small image, which seems to be getting scaled unexpectedly:

image

@SimonBrandner
Copy link
Contributor Author

Have you tested this with images smaller than the viewport? At the moment, this PR seems to still scale them, which I don't think we want.

In the original issue, you wrote: For small images that fit in the viewport, display them at 100% by default, and clicking the image is disabled. I think I misunderstood. My current understanding is that if the image is smaller than the viewport it should have the original size, right?

@jryans
Copy link
Collaborator

jryans commented Apr 26, 2021

In the original issue, you wrote: For small images that fit in the viewport, display them at 100% by default, and clicking the image is disabled. I think I misunderstood. My current understanding is that if the image is smaller than the viewport it should have the original size, right?

Ah sorry. If the image is smaller than the viewport, then it should appear as its original size, since there is no need to scale it at all. So to translate, "display them at 100%" means "show the image at 100% zoom" == "original size".

@jryans
Copy link
Collaborator

jryans commented Apr 26, 2021

Once the small image behaviour is fixed, I think this will be ready to review and merge. Thanks for working on this! 😄

@SimonBrandner
If the image is small don't scale
b741b31 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Try to precalculate the zoom from width and height props
dbca370 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner SimonBrandner marked this pull request as ready for review April 26, 2021 11:48
@SimonBrandner SimonBrandner requested a review from jryans April 26, 2021 11:50
jryans
jryans suggested changes Apr 26, 2021
View reviewed changes
Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

Overall this is looking great! 😄 Let's tune the cursor, and then I think it's ready to merge.

src/components/views/elements/ImageView.tsx Outdated Show resolved Hide resolved
src/components/views/elements/ImageView.tsx Show resolved Hide resolved
@SimonBrandner SimonBrandner requested a review from jryans April 26, 2021 13:13
@SimonBrandner
Show zoom buttons only if zooming is enabled
1fcad1a 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
i18n
38566a3 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
jryans
jryans approved these changes Apr 26, 2021
View reviewed changes
Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great, let's merge it! 😄

@jryans jryans merged commit 9401a6d into matrix-org:develop Apr 26, 2021
@jryans
Copy link
Collaborator

jryans commented Apr 26, 2021

@SimonBrandner Can you prepare a version that targets release-3.19.0 as well?

@SimonBrandner
Copy link
Contributor Author

@jryans, sure!

@SimonBrandner SimonBrandner mentioned this pull request Apr 26, 2021
Closed
@SimonBrandner SimonBrandner deleted the image-view-zoom branch April 26, 2021 15:24
@SimonBrandner SimonBrandner restored the image-view-zoom branch April 26, 2021 15:25
@jryans jryans mentioned this pull request Apr 26, 2021
Merged
@ara4n ara4n mentioned this pull request Aug 11, 2021
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jryans jryans jryans approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New image lightbox should zoom to 100% on image click
2 participants
@SimonBrandner @jryans