Perf images #319
Perf images #319
Conversation
| @@ -629,9 +629,7 @@ export const ampConfig: AMPConfig = { | |||
| "gtm-id": "GTM-XXXXXX", | |||
| "google-client-id-api": false, | |||
| "invalid-elements-strategy": "redirect-to-web-version", | |||
| "google-analytics-tracking-id": "UA-ABCDEFG", | |||
| "fallback-image-url": | |||
| "https://upload.wikimedia.org/wikipedia/commons/thumb/8/80/Wikipedia-logo-v2.svg/300px-Wikipedia-logo-v2.svg.png" | |||
There was a problem hiding this comment.
In the past we had thought of passing fallback-image-url via ampConfig using which publishers could pass an absolute url of a fallback image of their choice if they don't want to use the fallback image provided by ampLib (we didn't have a featureConfig back then). Removing it since we never added it to ampConfig, and I don't really see any publisher who would want this feature.
| switch (layout) { | ||
| case "fixed-height": | ||
| value.width = ""; |
There was a problem hiding this comment.
Found out during testing that images with layout = "fixed-height" should not have prop width.. it gives a validation error
There was a problem hiding this comment.
changed variable name from value to imgAttrs as it's more descriptive
| break; | ||
| case "fixed" || "intrinsic": |
There was a problem hiding this comment.
case "fixed" || "intrinsic" is invalid. Changed to
case "fixed":
case "intrinsic":
Found this on adding more tests
| config: Config; | ||
| slug: string; | ||
| slug: string | null; |
There was a problem hiding this comment.
image metadata and slug can be null. Made correction
src/atoms/image/image.tsx
Outdated
| slug, | ||
| aspectRatio, | ||
| alt, | ||
| layout = "responsive", | ||
| opts = {}, | ||
| srcSetOpts = {}, |
There was a problem hiding this comment.
opts -> used to add query params on src
srcSetOpts -> used to add query params on srcset
This is needed because hero-image needs to add {w: 1200, enlarge: true} only on src, not on srcset
src/atoms/image/image.test.tsx
Outdated
| ); | ||
| expect(wrapper.find("amp-img").exists()).toBeTruthy(); | ||
| expect(wrapper.find(`amp-img`).prop("srcset")).toBeUndefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
skipSrcset isn't being used anywhere, can remove if needed. Was planning to use it for PublisherLogoHeader component but decided not to use <Image> component in PublisherLogoHeader
| return { src, srcset }; | ||
| }; | ||
|
|
||
| export const focusedImagePath = ({ opts, slug, metadata, aspectRatio, cdnImage, width }: FocusedImagePathTypes) => { |
There was a problem hiding this comment.
Is this being used at any other place? If not, no need to export.
| skipSrcset | ||
| }: GetImgSrcAndSrcsetTypes) => { | ||
| const src = focusedImagePath({ opts, slug, metadata, aspectRatio, cdnImage }); | ||
| if (skipSrcset) return { src, srcset: null }; |
There was a problem hiding this comment.
When can we need to skip srcSet? Wondering will it ever be true
There was a problem hiding this comment.
#319 (comment)
Removing skipSrcset
|
|
||
| export const CoverPageBase = ({ story, config }: CoverPageProps) => { | ||
| const heroImgSrc = story["hero-image-s3-key"]; | ||
| const heroImgMetadata = story["hero-image-metadata"]; | ||
| const altText = story["hero-image-caption"] || story["hero-image-attribution"] || ""; | ||
| const authorNames = getAuthorNames(story.authors); | ||
| const imageAnimationProps = getImageAnimationProps(config); | ||
| const imgOpts = isGumlet(config) ? { format: "auto" } : {}; |
There was a problem hiding this comment.
Consider moving the logic of checking isGumlet true and adding format auto to opts and srcSetOpts to image component itself. Currently its duplicated across all the places.
This is unless you can think of any scenario when we would not want to not add format auto for a gumlet image.
There was a problem hiding this comment.
Ok, can move format: "auto" inside image
@ags1773 Can we get the comparison on vikatan-gamma before and after the changes? |
|
#313