-
Notifications
You must be signed in to change notification settings - Fork 613
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
STENCIL-3106 Switch from jquery-zoom to easyzoom #1096
Conversation
lgtm 👍 |
this.setActiveThumb(); | ||
this.swapMainImage(); | ||
this.setImageZoom(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we still need to call setImageZoom
. otherwise this.easyzoom
will be undefined in swapMainImage
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe we need to, as this zoom library works a bit differently than the old one. Now you just initialize it on an element (in this case the productView-image figure), and it manages swapping both the zoomed image and the normal-sized images. So the same easyzoom instance is actually sticking around as you swap images around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, i see - i completely missed that it's called from the init
method.
|
||
destroyImageZoom() { | ||
this.$mainImage.trigger('zoom.destroy'); | ||
this.easyzoom = $('.easyzoom').easyZoom({errorNotice: "", loadingNotice: ""}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm thinking we can scope the call to $('.easyzoom')
here like this.$mainImage.$('.easyzoom')
so it doesn't need to traverse the whole dom
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, there's actually no need for the .easyzoom class at all, so I just removed it entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
69b01b5
to
b14d867
Compare
…agnified image loading
b14d867
to
f223a0a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@BC-EChristensen this is missing a changelog entry. Can you please add it as part of #1097 |
Ack, sorry about that. Yes, will add it to #1097. |
Thanks for this! Excited to see the improvements. |
What?
Allow for lazy loading of the magnified "zoom" image on the product details page, by using the EasyZoom library instead of jquery-zoom.
Tickets / Documentation