Integrate srcSet #693

aChanEP · 2020-05-07T02:52:34Z

Description:

https://elasticpath.atlassian.net/secure/RapidBoard.jspa?rapidView=467&projectKey=RS&modal=detail&selectedIssue=RS-805&assignee=557058%3A2cdf4e37-eb83-44ee-9d76-d5409db1e998

The purpose of this PR is to implement dynamic images that will fetch the appropriate asset based on window width and browser compatibility. This PR will stop lighthouse from complaining about using next-gen formats and improper sizing for our images. lighthouse performance sees modest improvements and we are hitting the mid to high 50's.

ImageContainer component has been updated to utilize dynamic images enabled through the picture element. Take a look at the component README for more information.

Image assets for the Breville catalog have been sized appropriately for 3 sizing breakpoints: 0-768, 768 - 1092, 1092 - Infinity. Each sizing containing jpg, jp2, and webp formats which will cover a full range of browsers. The assets can be seen at this s3 bucket: https://s3.console.aws.amazon.com/s3/buckets/refstoreimgs/?region=us-west-2&tab=overview.

The demo-team will need to be notified of these assets changes as they are currently using pngs for their demo's.

A seperate project here for image processing:
https://github.com/aChanEP/nextGenImageCreation. We may want to use this as a starting point for a future asset pipeline if we deem it necessary.

// Processed images for bellevie dataset
"skuImagesUrl": "https://ep-demo-assets.s3-us-west-2.amazonaws.com/BELLEVIE/skuImages/%fileName%",
"siteImagesUrl": "https://ep-demo-assets.s3-us-west-2.amazonaws.com/BELLEVIE/siteImages/%fileName%",

TODO's:

Still needing to debug multi-sku images. DONE
Need to write up documentation on how to reskin. WIP

Linting:

No linting errors

Tests:

E2E tests (npm test run with e2e)
Manual tests

Documentation:

Requires documentation updates

Integrate srcSet

Able to get srcSet working

change to use x instead of w

adjust sizings

clean up

Able to get the homepage to use srcSet

Able to get pdp to work with new image container

able to get appheader working

able to get image container working in productlistitem

uncomment all usages of image container

add styling

update ep.config.json

update mandatory alt tag for img element

More updates to alt element. Include better descriptions for imaeg parameters

fix readme and make more descriptive parameter names

update config file

update README example

shaunmaharaj · 2020-05-12T17:24:00Z

Please address conflicts & rebase

shaunmaharaj · 2020-05-12T17:24:28Z

src/ep.config.json

@@ -27,12 +25,12 @@
    }
  ],
  "defaultCurrencyValue": "CAD",
-  "skuImagesUrl": "https://elasticpath-demo-images.s3-us-west-2.amazonaws.com/VESTRI_VIRTUAL_V1/%sku%.png",
-  "siteImagesUrl": "https://elasticpath-demo-images.s3-us-west-2.amazonaws.com/VESTRI_VIRTUAL_V1/%fileName%",
+  "skuImagesUrl": "https://refstoreimgs.s3-us-west-2.amazonaws.com/productImages/%fileName%",


Is this a copy of the existing bucket's images?

Yes its a copy of existing buckets. resized and re-typed versions of the breville catalog within the reference store aws

shaunmaharaj · 2020-05-12T17:24:52Z

src/ep.config.json

-  "skuImagesUrl": "https://elasticpath-demo-images.s3-us-west-2.amazonaws.com/VESTRI_VIRTUAL_V1/%sku%.png",
-  "siteImagesUrl": "https://elasticpath-demo-images.s3-us-west-2.amazonaws.com/VESTRI_VIRTUAL_V1/%fileName%",
+  "skuImagesUrl": "https://refstoreimgs.s3-us-west-2.amazonaws.com/productImages/%fileName%",
+  "siteImagesUrl": "https://refstoreimgs.s3-us-west-2.amazonaws.com/siteImages/%fileName%",


Getting CORS errors on these images. Need to add a CORS config for the new bucket:

Updated the url

FYI I get cors issues even when switching back to elasticpath-demo-images

shaunmaharaj · 2020-05-12T17:25:59Z

src/components/src/ProductListItem/productlistitem.main.tsx

+                imgClassName="category-item-thumbnail img-responsive"
+                isSkuImage
+                fileName={productData._code[0].code}
+                imageFileTypes={['webp', 'jp2', 'jpg']}


Can we take the "imageFileTypes" from ep.config.json? We're duplicating this across components but it's a shared config and could be changed by an implementor.

I would +1 this!

Would it be possible to make the imageFileTypes property optional, and have it override whatever is in ep.config.json?

Most Demo Team assets are PNGs. As this stands right now, we're going to need to not only convert our existing assets to one of the above formats, but also create duplicates (in each of the three formats) for every demo we put out (or go through and manually update the types on each of these components). It would definitely reduce friction if we could configure this in one place, and override manually where necessary.

^ Overriding it per usage sounds like a good idea to me! I'll make those changes :)

src/ep.config.json

src/containers/MaintenancePage.tsx

Remove image type input

plundaahl-ep · 2020-05-12T22:18:17Z

src/components/src/OrderTableLineItem/ordertable.lineitem.tsx

+          imgClassName="thumbnail"
+          isSkuImage
+          fileName={code}
+          imageFileTypes={['webp', 'jp2', 'jpg']}


Would it be worth pulling this out before merging this in, and letting the details in ep.config.json determine the types used here?

Also, same question about the following files:

src/components/src/QuickOrderForm/quickorderform.tsx

src/components/src/ProductListItem/productlistitem.main.tsx

src/components/src/ProductDisplayItem/productdisplayitem.main.tsx

Yep removing these types. Will be an optional parameter that will override the types in the config.

dusanradovanovic · 2020-05-13T19:40:24Z

src/components/src/ImageContainer/image.container.tsx

  isSkuImage: false,
+  ImageContainerSrcs: [],


I don't see this defined in the props interface. Where is it used? If you want to keep it it should be camel case like all the prop names.

good catch. Removing

remove alt

change to use React Component

remove styling

remove useless onerror

add newline

get rid of alt

update readme

new line

aChanEP

Please address conflicts & rebase

rebased

aChanEP · 2020-05-13T14:53:00Z

src/components/src/ImageContainer/image.container.scss

@@ -0,0 +1,3 @@
+picture {
+    width: 100%;
+}


good catch!

src/components/src/ImageContainer/image.container.tsx

aChanEP · 2020-05-13T14:53:42Z

src/components/src/ImageContainer/image.container.tsx

-  );
+  const generateSrcSet = type => imageSizes.reduce((acc, imageSize) => {
+    if (acc === '') {
+      return `${acc}${imgPrefix.replace('%fileName%', `${type}/${fileName}-${imageSize}w.${type} ${imageSize}w`)}`;


WIP looking into it

src/components/src/ImageContainer/image.container.tsx

aChanEP · 2020-05-13T19:43:42Z

src/components/src/ImageContainer/image.container.tsx

  isSkuImage: false,
+  ImageContainerSrcs: [],


good catch. Removing

src/components/src/ImageContainer/image.container.tsx

src/ep.config.json

support mutli-sku images

update config

Update storybook README and stories

use cleaner approach

update image container styling

add aria-label back

add new line

aChanEP · 2020-05-14T21:30:32Z

All problems have been addressed here! This should be ready to be merged! Working on re-skinning docs now :)

src/ep.config.json

update README and make slight changes to error handling

change naming

stop never ending refresh loop

make some quick fixes

shaunmaharaj · 2020-05-16T19:04:20Z

src/components/src/ProductListItem/productlistitem.main.tsx

@@ -147,8 +147,14 @@ class ProductListItemMain extends Component<ProductListItemMainProps, ProductLis
      return (
        <div className="category-item-inner">
          <div className={`category-item-thumbnail-container ${imageStatus === 'loaded' ? 'loaded' : ''}`}>
-            <Link to={`${itemDetailLink}/${encodeURIComponent(productData._code[0].code)}`} title={productData._definition[0]['display-name']}>


axe reports an error due to the missing title. I'll fix this in my PR with the version update before release cut-off:
#712

aChanEP added 17 commits May 6, 2020 21:51

ddef445

Integrate srcSet

7d89bde

Able to get srcSet working

fc48651

change to use x instead of w

41fe978

adjust sizings

8a4f6ab

clean up

12e9637

Able to get the homepage to use srcSet

06a3a3f

Able to get pdp to work with new image container

4fb8a02

able to get appheader working

f6faf61

able to get image container working in productlistitem

f4c0512

uncomment all usages of image container

8986cbe

add styling

175e700

update ep.config.json

63584b6

update mandatory alt tag for img element

a3ce059

More updates to alt element. Include better descriptions for imaeg parameters

ced6925

fix readme and make more descriptive parameter names

c5de622

update config file

665444f

update README example

aChanEP requested review from shaunmaharaj and dusanradovanovic May 12, 2020 16:47

aChanEP changed the title ~~[WIP] Integrate srcSet~~ Integrate srcSet May 12, 2020

shaunmaharaj reviewed May 12, 2020

View reviewed changes

src/ep.config.json Show resolved Hide resolved

shaunmaharaj reviewed May 12, 2020

View reviewed changes

src/ep.config.json Show resolved Hide resolved

shaunmaharaj reviewed May 12, 2020

View reviewed changes

src/containers/MaintenancePage.tsx Show resolved Hide resolved

aChanEP added 2 commits May 12, 2020 17:36

d52f4f1

Remove image type input

add types back

8f4ae4c

plundaahl-ep reviewed May 12, 2020

View reviewed changes

dusanradovanovic reviewed May 13, 2020

View reviewed changes

aChanEP added 8 commits May 13, 2020 17:12

71009af

remove alt

2c65311

change to use React Component

907bb18

remove styling

0abab5d

remove useless onerror

07f6af0

add newline

d465bd7

get rid of alt

0f3f480

update readme

9df9f6f

new line

aChanEP commented May 13, 2020

View reviewed changes

shaunmaharaj reviewed May 14, 2020

View reviewed changes

src/ep.config.json Outdated Show resolved Hide resolved

aChanEP added 7 commits May 14, 2020 13:42

5db39bf

support mutli-sku images

d341f8c

update config

bd24184

Update storybook README and stories

37e3e3a

use cleaner approach

ba2d613

update image container styling

cb4ce0e

add aria-label back

835d720

add new line

shaunmaharaj reviewed May 14, 2020

View reviewed changes

src/ep.config.json Outdated Show resolved Hide resolved

elasticpath deleted a comment from allcontributors bot May 14, 2020

aChanEP added 3 commits May 14, 2020 18:38

17fa7fe

update README and make slight changes to error handling

405dff5

change naming

babc9e3

stop never ending refresh loop

dusanradovanovic approved these changes May 14, 2020

View reviewed changes

shaunmaharaj approved these changes May 15, 2020

View reviewed changes

69f4ff4

make some quick fixes

aChanEP merged commit 40bfd67 into master May 15, 2020

aChanEP deleted the srcSet branch May 15, 2020 15:37

shaunmaharaj reviewed May 16, 2020

View reviewed changes

Integrate srcSet #693

Integrate srcSet #693

Uh oh!

Conversation

aChanEP commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shaunmaharaj commented May 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plundaahl-ep May 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plundaahl-ep May 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aChanEP left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aChanEP commented May 14, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aChanEP commented May 7, 2020 •

edited

Loading

plundaahl-ep May 12, 2020 •

edited

Loading

plundaahl-ep May 12, 2020 •

edited

Loading