-
Notifications
You must be signed in to change notification settings - Fork 615
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-3107 - don't load images below the fold #944
Conversation
Autotagging @mcampa @bc-miko-ademagic @davidchin |
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.
Hey @bookernath, nice work. Can you take a look at my comments?
assets/js/theme/global/lazysizes.js
Outdated
@@ -0,0 +1,4 @@ | |||
import 'lazysizes'; |
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.
If lazysizes
module gets initialized automatically, you probably don't need this file. Just import 'lazysizes
directly in assets/js/theme/global.js
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.
addressed.
@@ -17,7 +17,7 @@ | |||
{{/if}} | |||
{{/or}} | |||
<a href="{{url}}"> | |||
<img class="card-image" src="{{getImage image 'productgallery_size' (cdn theme_settings.default_image_product)}}" alt="{{image.alt}}" title="{{image.alt}}"> | |||
<img class="card-image lazyload" data-sizes="auto" src="{{cdn 'img/loading.gif'}}" data-src="{{getImage image 'productgallery_size' (cdn theme_settings.default_image_product)}}" alt="{{image.alt}}" title="{{image.alt}}"> |
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.
Can you double check to make sure the loading gif looks good in the other variations? I think bold and warm have a non-white background. So the gif might have jagged white edges around the spinning circle.
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.
good callout.
I've switched to SVG - this won't animate on IE, but I picked a type of icon which should still clearly convey to a user that an asset is loading. On most other browsers it will animate as intended.
I think this is a good compromise after thinking about it quite a bit.
assets/img/loading.svg
Outdated
@@ -0,0 +1 @@ | |||
<svg width='50px' height='50px' xmlns="http://www.w3.org/2000/svg" viewBox="0 0 100 100" preserveAspectRatio="xMidYMid" class="uil-default"><rect x="0" y="0" width="100" height="100" fill="none" class="bk"></rect><rect x='46.5' y='40' width='7' height='20' rx='5' ry='5' fill='#2d2d2d' transform='rotate(0 50 50) translate(0 -30)'> <animate attributeName='opacity' from='1' to='0' dur='1s' begin='0s' repeatCount='indefinite'/></rect><rect x='46.5' y='40' width='7' height='20' rx='5' ry='5' fill='#2d2d2d' transform='rotate(30 50 50) translate(0 -30)'> <animate attributeName='opacity' from='1' to='0' dur='1s' begin='0.08333333333333333s' repeatCount='indefinite'/></rect><rect x='46.5' y='40' width='7' height='20' rx='5' ry='5' fill='#2d2d2d' transform='rotate(60 50 50) translate(0 -30)'> <animate attributeName='opacity' from='1' to='0' dur='1s' begin='0.16666666666666666s' repeatCount='indefinite'/></rect><rect x='46.5' y='40' width='7' height='20' rx='5' ry='5' fill='#2d2d2d' transform='rotate(90 50 50) translate(0 -30)'> <animate attributeName='opacity' from='1' to='0' dur='1s' begin='0.25s' repeatCount='indefinite'/></rect><rect x='46.5' y='40' width='7' height='20' rx='5' ry='5' fill='#2d2d2d' transform='rotate(120 50 50) translate(0 -30)'> <animate attributeName='opacity' from='1' to='0' dur='1s' begin='0.3333333333333333s' repeatCount='indefinite'/></rect><rect x='46.5' y='40' width='7' height='20' rx='5' ry='5' fill='#2d2d2d' transform='rotate(150 50 50) translate(0 -30)'> <animate attributeName='opacity' from='1' to='0' dur='1s' begin='0.4166666666666667s' repeatCount='indefinite'/></rect><rect x='46.5' y='40' width='7' height='20' rx='5' ry='5' fill='#2d2d2d' transform='rotate(180 50 50) translate(0 -30)'> <animate attributeName='opacity' from='1' to='0' dur='1s' begin='0.5s' repeatCount='indefinite'/></rect><rect x='46.5' y='40' width='7' height='20' rx='5' ry='5' fill='#2d2d2d' transform='rotate(210 50 50) translate(0 -30)'> <animate attributeName='opacity' from='1' to='0' dur='1s' begin='0.5833333333333334s' repeatCount='indefinite'/></rect><rect x='46.5' y='40' width='7' height='20' rx='5' ry='5' fill='#2d2d2d' transform='rotate(240 50 50) translate(0 -30)'> <animate attributeName='opacity' from='1' to='0' dur='1s' begin='0.6666666666666666s' repeatCount='indefinite'/></rect><rect x='46.5' y='40' width='7' height='20' rx='5' ry='5' fill='#2d2d2d' transform='rotate(270 50 50) translate(0 -30)'> <animate attributeName='opacity' from='1' to='0' dur='1s' begin='0.75s' repeatCount='indefinite'/></rect><rect x='46.5' y='40' width='7' height='20' rx='5' ry='5' fill='#2d2d2d' transform='rotate(300 50 50) translate(0 -30)'> <animate attributeName='opacity' from='1' to='0' dur='1s' begin='0.8333333333333334s' repeatCount='indefinite'/></rect><rect x='46.5' y='40' width='7' height='20' rx='5' ry='5' fill='#2d2d2d' transform='rotate(330 50 50) translate(0 -30)'> <animate attributeName='opacity' from='1' to='0' dur='1s' begin='0.9166666666666666s' repeatCount='indefinite'/></rect></svg> |
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.
Do we need a new line at the end ?
Hey @bookernath, thanks for doing the changes. I'm still not sure about the SVG spinner, because, AFAIK, it doesn't work in IE at all (even in Edge). I'm wondering if we need to use a spinner at all? I think a simple fade-in effect might work well? Considering you're only lazy loading small product images and they generally load pretty fast, you probably don't want a dozen spinners cluttering the page (the design of that spinner is particularly clunky). What do you think? |
@davidchin I do think the icon is important so that the elements in the DOM take up a little space and also so that users know to expect content to load there - but I'm more than happy to use a different icon. |
Implementing lazyload to prevent images below the fold from loading before they’re needed. Focus is on product card images.
Updated with a new icon that's a little more on-brand, similar to loading animations we use in the CP. I've confirmed it looks OK on Bold. I've also confirmed it'll render as a static image on IE, which should still convey the right idea. :) |
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.
Thanks @bookernath. I like the new icon, it's definitely nicer. It's a shame SVG animation is not supported in Edge for the moment. But it will probably be supported soon. So it's a good compromise.
Sorry about the delay on this one, but I finally got a clean regression run. Did some manual clicking around as well... LGTM 💚 |
What?
Implementing lazyload to prevent images below the fold from loading
before they’re needed. Focus is on product card images.
Tickets / Documentation
Screenshots (if appropriate)
Currently live on https://bigcommerce.support/ for testing