Clarifying comments + more srcset values

ampproject · May 28, 2020 · 1c8ae22 · 1c8ae22
1 parent 1196fdd
commit 1c8ae22
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 6 deletions.
diff --git a/packages/optimizer/lib/transformers/OptimizeImages.js b/packages/optimizer/lib/transformers/OptimizeImages.js
@@ -24,13 +24,37 @@ const {isValidImageSrcURL} = require('../URLUtils');
 // this avoids generating srcsets for images with a responsive layout where width/height define the aspect ration.
 const MIN_WIDTH_TO_ADD_SRCSET_IN_RESPONSIVE_LAYOUT = 100;
 
-// All legimate srcset widths.
-const SRCSET_WIDTH = [39, 56, 82, 100, 150, 300, 500, 750, 1000, 1500, 2000, 2500];
+// All supported srcset widths.
+const SRCSET_WIDTH = [
+  39,
+  47,
+  56,
+  68,
+  82,
+  100,
+  120,
+  150,
+  180,
+  220,
+  270,
+  330,
+  390,
+  470,
+  560,
+  680,
+  820,
+  1000,
+  1200,
+  1440,
+  1750,
+  2000,
+  2500,
+];
 
 // Don't generate srcsets for images larger than the supported maximum
 const MAX_IMG_SIZE = SRCSET_WIDTH[SRCSET_WIDTH - 1];
 
-// The maximum number of values. We'll take the initial image width and generate more width values by
+// The maximum number of srcset source. We'll take the initial image width and generate more width values by
 // multiplying by multiples of 1.0 up the given max value (e.g. width=300, maxSrcsetValues=3 => 1 * 300, 2 * 300, 3 * 300)
 // and match the result to the closest supported srcset width (see above).
 const MAX_SRCSET_VALUE_COUNT = 3;
@@ -122,7 +146,7 @@ class OptimizeImages {
     this.log = config.log;
     this.imageOptimizer = config.imageOptimizer;
     this.srcsetWidth = new SrcsetWidth();
-    // TODO turn these into options
+    // TODO turn these into options https://github.com/ampproject/amp-toolbox/issues/804
     this.maxImageWidth = MAX_IMG_SIZE;
     this.maxSrcsetValues = MAX_SRCSET_VALUE_COUNT;
   }

diff --git a/packages/optimizer/spec/transformers/valid/OptimizeImages/adds_srcset/expected_output.html b/packages/optimizer/spec/transformers/valid/OptimizeImages/adds_srcset/expected_output.html
@@ -1,13 +1,13 @@
 <html ⚡>
 <body>
   <!-- Injects srcset for responsive layout -->
-  <amp-img src="image1.png" width="400" height="800" layout="responsive" srcset="image1.500w.png 500w, image1.1000w.png 1000w, image1.1500w.png 1500w"></amp-img>
+  <amp-img src="image1.png" width="400" height="800" layout="responsive" srcset="image1.470w.png 470w, image1.820w.png 820w, image1.1440w.png 1440w"></amp-img>
   <!-- Ignores existing srcset -->
   <amp-img layout="fill" srcset="image-1x.png 1x, 
                                  image-2x.png 2x"></amp-img>
   <!-- Only adds srcset if # images >2  -->
   <amp-img layout="responsive" width="2400" height="600" src="https://unsplash.it/1600/1200"></amp-img>
   <!-- Injects srcset for intrinsic layout -->
-  <amp-img layout="intrinsic" width="20" height="10" src="https://unsplash.it/1600/1200" srcset="https://unsplash.39w.it/1600/1200 39w, https://unsplash.56w.it/1600/1200 56w, https://unsplash.82w.it/1600/1200 82w"></amp-img>
+  <amp-img layout="intrinsic" width="20" height="10" src="https://unsplash.it/1600/1200" srcset="https://unsplash.39w.it/1600/1200 39w, https://unsplash.47w.it/1600/1200 47w, https://unsplash.68w.it/1600/1200 68w"></amp-img>
 </body>
 </html>