Fix scroll to zoom, resize small images to fill #1063
Conversation
skjnldsv
added
3. to review
skjnldsv
added
2. developing
|
@skjnldsv Can you please take a look at my replies (in particular the one about the hover background) Also I'm assuming the |
Yes :)
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
What the |
Setting the background to white is removed since it doesn't work well with the zooming (Outside the image boundaries is still transparent) Signed-off-by: ktprograms <[email protected]>
Add window resize event listener to check if image is filling width or height and then set either (width,max-height) or (height,max-width) such that small images are still resized to fill the view area. Signed-off-by: ktprograms <[email protected]>
|
@skjnldsv 70957b6 is because I set up Edit: 9407116 (turns out I didn't have |
|
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 |
Signed-off-by: ktprograms <[email protected]>
There was a problem hiding this comment.
Ok, so this is much better already.
I like it actually, nice approach. There is some regression though and this is the very reason position: absolute was not being used in the first place.
Initially, we did NOT want to enlarge small pictures. Which currently, it is done by that PR. This is the only regression found when testing all the use cases I have with images sized 👍
Here is the list of other regressions:
- Small pictures are enlarged
- Arrows are over the pictures
- Title is over the picture (bright image will hide the title)
@jancborchardt @nimishavijay needs to validate this before we merge!
I'm personally ok with that if we don't put the image under the title.
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).