Srcset zoom #51
Srcset zoom #51
Conversation
There was a problem hiding this comment.
This is awesome! I believe it solves everything that has been discussed in #27, right?
I left a few notes mainly on the development environment (that needs to be reworked on soon because it's dodgy).
examples/demo/demo.js
Outdated
| @@ -28,7 +28,7 @@ | |||
|
|
|||
| // Add zooms to a container | |||
| var containerZoom = [ | |||
| mediumZoom('#zoom-default'), | |||
| mediumZoom('.zoom-default'), | |||
There was a problem hiding this comment.
We shouldn't edit this line since it removes the zoom of the actual demo (the one in production and not in development). We need to think of a better way to handle the different environments of the library. I've been thinking about that for a long time now but haven't got a great solution yet (we can open another issue for that).
For now, I suggest we keep mediumZoom('#zoom-default') here and add the following code snippet line 28 (we don't need to add it to containerZoom because it is only for development purpose):
mediumZoom('.zoom-srcset')
README.md
Outdated
| @@ -345,6 +345,14 @@ zoom.addEventListeners('show', event => { | |||
| }) | |||
| ``` | |||
|
|
|||
| ## Compatibility | |||
There was a problem hiding this comment.
This shouldn't be in this PR and needs to be reverted: git revert 2433e3d6546ee274234e9b2080724e02964902c4.
There was a problem hiding this comment.
Sorry for that, sneaked in somehow
examples/demo/development/index.html
Outdated
| <figure> | ||
| <img | ||
| id="zoom-default" | ||
| class="zoom-default" |
There was a problem hiding this comment.
We can remove this class.
examples/demo/development/index.html
Outdated
|
|
||
| <figure> | ||
| <img | ||
| id="zoom-srcset-default" |
examples/demo/development/index.html
Outdated
| <figure> | ||
| <img | ||
| id="zoom-srcset-default" | ||
| class="zoom-default" |
There was a problem hiding this comment.
Change the class to zoom-srcset.
examples/demo/development/index.html
Outdated
|
|
||
| <figure style="margin-right: auto; margin-left: auto; max-width: 400px"> | ||
| <img | ||
| id="zoom-srcset-dtz-default" |
examples/demo/development/index.html
Outdated
| <figure style="margin-right: auto; margin-left: auto; max-width: 400px"> | ||
| <img | ||
| id="zoom-srcset-dtz-default" | ||
| class="zoom-default" |
There was a problem hiding this comment.
Change the class to zoom-srcset.
src/medium-zoom.js
Outdated
| // is already in cache | ||
| const loadEventListener = target.zoomedHd.addEventListener('load', () => { | ||
| // Clean up after ourselfes | ||
| target.zoomedHd.removeEventListener('load', loadEventListener); |
There was a problem hiding this comment.
Semicolon (;) should be omitted.
|
Thank you for the new changes @tuelsch! We need to revert the changes in the README and we're ready to merge! |
This reverts commit 2433e3d.
|
🎉 Medium Zoom 1.0.0 has been released @tuelsch! |
This PR tries to fix the srcset zoom problem as discussed in #27. It behaves like loading a hd image with
data-zoom-targetand starts the zoom after the appropriate image is being loaded. I used theloadevent instead of polling with an interval because in a quick local test with throttled network the load event and the interval fired at similar times with a different winner every time. Also, theloadevent fires nicely when the image is already cached.There are a ton of possibilities for configuring srcset and sizes so I did not spend the time to test every single one of them, allthough I think the two cases in the demo html are pretty common ones.
Closes #27.