STENCIL-3891 Lazy load images, save space for images while loading #1104

christensenep · 2017-10-17T17:04:47Z

What?

Lazy load all site images, and save space for images while they are loading.

Tickets / Documentation

bookernath · 2017-10-17T17:10:20Z

Nice work! Any idea what the browser compatibility looks like for some of the CSS you're using?

christensenep · 2017-10-17T17:29:21Z

I don't believe I'm using anything that will affect compatibility, but one of the reasons I have it marked as WIP is indeed that I want to take it through the browserstack ringer first.

junedkazi · 2017-10-20T16:28:22Z

assets/scss/components/stencil/cart/_cart.scss

    text-align: center;
+    position: relative;
+
+    img {


Is this a good candidate for a @mixin since it is used in a couple of places.

junedkazi · 2017-10-20T16:28:52Z

assets/scss/components/stencil/writeReview/_writeReview.scss

+    content: '';
+    display: block;
+    padding-bottom: get-padding(stencilString('product_size'));
+}


EOL is missing

junedkazi · 2017-10-20T16:29:00Z

assets/scss/components/citadel/loading/_loading.scss

+
+.lazyload, .lazyloading {
+    height:100%;
+}


EOL is missing

junedkazi · 2017-10-20T16:30:33Z

assets/scss/components/stencil/productView/_productView.scss

@@ -72,7 +90,7 @@
        position: absolute;
        right: 0;
        top: 0;
-        width: auto;
+        width: 100%;


won't this stretch the image to the width of the outside container. This could cause image to stretch when they are loaded with a small dimension like 10x10

Oh, I remembered why I did this. There's currently an issue when the browser width is extremely small, the thumbnails will extend outside their borders, like so:

I would have liked to use max-width, but unfortunately we're already setting max-width at 50px here. I don't know of a CSS trick to essentially say "max of 50px or 100%, whichever is less", but perhaps there is one.

In any event, this doesn't really have to be a part of this ticket, so I changed it back.

junedkazi · 2017-10-20T16:30:58Z

assets/scss/layouts/brands/_brand.scss

+  .card-img-container:after {
+    padding-bottom: get-padding(stencilString('brand_size'));
+  }
+}


EOL is missing

junedkazi · 2017-10-20T16:31:14Z

assets/scss/tools/_image.scss

+	$list: str-split($size, 'x');
+
+    @return nth($list, 2) + 'px';
+}


EOL is missing

junedkazi · 2017-10-20T16:32:54Z

assets/scss/tools/_image.scss

@@ -0,0 +1,21 @@
+// http://sassmeister.com/gist/1b4f2da5527830088e4d
+@function get-padding($size) {


Can we add some doc blocks for these helpers which we are adding. Also this would be a good candidate for documentation.

junedkazi · 2017-10-20T16:33:12Z

assets/scss/tools/_string.scss

@@ -8,3 +8,66 @@

    @return $string;
 }
+
+// https://stackoverflow.com/questions/32376461/how-to-split-a-string-into-two-lists-of-numbers-in-sass
+@function str-split($string, $separator) {


Can we add some doc blocks for these helpers which we are adding. Also this would be a good candidate for documentation.

junedkazi · 2017-10-20T16:33:22Z

assets/scss/tools/_string.scss

+  }
+
+  @return $value * map-get($units, $unit);
+}


EOL is missing

junedkazi · 2017-10-20T16:33:29Z

assets/scss/tools/_tools.scss

@@ -1,2 +1,3 @@
 @import "list";
 @import "string";
+@import "image";


EOL is missing

junedkazi · 2017-10-20T16:38:11Z

assets/scss/tools/_string.scss

+}
+
+// https://www.sassmeister.com/gist/9fa19d254864f33d4a80
+@function to-number($value) {


Since we are adding all these helpers should we think about adding some unit tests to go with it as well. This is just for the team to think over. Not a priority for this PR @bigcommerce/stencil-team

junedkazi · 2017-10-20T16:39:45Z

CHANGELOG.md

@@ -2,6 +2,8 @@

 ## Draft
 - Use appropriately-sized (50x50) images for product thumbnails on product details page [#1097](https://github.com/bigcommerce/cornerstone/pull/1097)
+- Load visible images before images below the fold [#1104](https://github.com/bigcommerce/cornerstone/pull/1104)
+- Save space for lazy loading images while they are loading [#1104](https://github.com/bigcommerce/cornerstone/pull/1104)


I am not sure we should be adding multiple lines pointing to the same PR. Can we update the desc in the first one to include the second one as well.

tabayomi · 2017-10-24T22:04:25Z

assets/scss/components/stencil/productView/_productView.scss

-        margin-top: spacing("half");
-    }
+.productView-img-container:after {
+    content: '';


just a helpful pointer - a few other places lend themselves well to using mixins

tabayomi · 2017-10-24T22:20:54Z

assets/scss/components/foundation/lazyLoad/_lazyLoad.scss

+  position: absolute;
+  top: 0;
+  bottom: 0;
+  left: 0;


Indentation - 4 soft tabs - see style guide

tabayomi · 2017-10-24T22:22:24Z

assets/scss/layouts/brands/_brand.scss

+.brand-image-container:after {
+  content: '';
+  display: block;
+  padding-bottom: get-padding(stencilString('thumb_size'));


Indentation - 4 soft tabs

tabayomi · 2017-10-24T22:58:59Z

assets/scss/tools/_string.scss

+@function to-number($value) {
+  @if type-of($value) == 'number' {
+    @return $value;
+  } @else if type-of($value) != 'string' {


Indentation

tabayomi · 2017-11-01T23:06:06Z

Looks good to me 👍

shanth100 · 2017-11-07T23:07:10Z

💚

bigbot · 2017-11-09T21:56:49Z

Autotagging @bigcommerce/stencil-team @davidchin

christensenep force-pushed the STENCIL-3891 branch from 9b7d57b to aca239a Compare October 17, 2017 17:27

christensenep force-pushed the STENCIL-3891 branch 5 times, most recently from 1e34b23 to 7605c95 Compare October 18, 2017 18:42

christensenep changed the title ~~[WIP] STENCIL-3891 Lazy load images, save space for images while loading~~ STENCIL-3891 Lazy load images, save space for images while loading Oct 18, 2017

christensenep force-pushed the STENCIL-3891 branch 7 times, most recently from e87dc94 to 6fdb08f Compare October 19, 2017 18:46