Skip to content
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

Use img tag instead of picture #2067

Merged
merged 12 commits into from
Dec 4, 2020
4 changes: 2 additions & 2 deletions .storybook/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@ import { defaults } from './default-css';
import 'reset-css';

import { Lazy } from '@root/src/web/components/Lazy';
import { Picture } from '@root/src/web/components/Picture';
import { Img } from '@root/src/web/components/Img';

// Prevent components being lazy rendered when we're taking Chromatic snapshots
Lazy.disabled = isChromatic();
Picture.disableLazyLoading = isChromatic();
Img.disableLazyLoading = isChromatic();

// Add base css for the site
let css = `${getFontsCss()}${defaults}`;
Expand Down
13 changes: 8 additions & 5 deletions cypress/integration/e2e/consent.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ describe('Consent tests', function () {

const cmpIframe = () => {
return cy
.get('iframe[id*="sp_message_iframe"]')
.get('iframe[id*="sp_message_iframe"]', { timeout: 30000 })
.its('0.contentDocument.body')
.should('not.be.empty')
.then(cy.wrap);
Expand All @@ -31,7 +31,7 @@ describe('Consent tests', function () {
// after the pageLoadEvent has fired
// eslint-disable-next-line cypress/no-unnecessary-waiting
cy.wait(300);
}
};

beforeEach(function () {
// Reset CMP cookies before each test
Expand Down Expand Up @@ -64,7 +64,9 @@ describe('Consent tests', function () {
cmpIframe().find("[title='Manage my cookies']").click();
// Reject tracking cookies
privacySettingsIframe().contains('Privacy settings');
privacySettingsIframe().find("[title='Reject all']", {timeout: 30000}).click();
privacySettingsIframe()
.find("[title='Reject all']", { timeout: 30000 })
.click();
// Make a second page load now that we have the CMP cookies set to reject tracking and check
// to see if the ga property was set by Google on the window object
cy.visit(`Article?url=${secondPage}`);
Expand All @@ -82,12 +84,13 @@ describe('Consent tests', function () {
cmpIframe().find("[title='Manage my cookies']").click();
// Reject tracking cookies
privacySettingsIframe().contains('Privacy settings');
privacySettingsIframe().find("[title='Accept all']", {timeout: 30000}).click();
privacySettingsIframe()
.find("[title='Accept all']", { timeout: 30000 })
.click();
// Make a second page load now that we have the CMP cookies set to reject tracking and check
// to see if the ga property was set by Google on the window object
cy.visit(`Article?url=${secondPage}`);
waitForAnalyticsToInit();
cy.window().its('ga').should('exist');
});

});
107 changes: 107 additions & 0 deletions src/web/components/Img.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
import React from 'react';
import { css } from 'emotion';

import { breakpoints } from '@guardian/src-foundations/mq';

type Props = {
imageSources: ImageSource[];
role: RoleType;
alt: string;
height: string;
width: string;
isMainMedia?: boolean;
isLazy?: boolean;
};

const getClosestSetForWidth = (
desiredWidth: number,
inlineSrcSets: SrcSetItem[],
): SrcSetItem => {
// For a desired width, find the SrcSetItem which is the closest match
const sorted = inlineSrcSets.sort((a, b) => b.width - a.width);
return sorted.reduce((best, current) => {
if (current.width < best.width && current.width >= desiredWidth) {
return current;
}
return best;
});
};

const getFallbackSrc = (srcSets: SrcSetItem[]): string => {
// The assumption here is readers on devices that do not support srcset are likely to be on poor
// network connections so we're going to fallback to a small image
return getClosestSetForWidth(300, srcSets).src;
};

const getSrcSetsForRole = (
role: RoleType,
imageSources: ImageSource[],
): SrcSetItem[] => {
return imageSources.filter(
({ weighting }) =>
// Use toLowerCase to handle cases where we have halfWidth comparing to halfwidth
weighting.toLowerCase() === role.toLowerCase(),
)[0].srcSet;
};

const buildSourcesString = (srcSets: SrcSetItem[]): string => {
return srcSets.map((srcSet) => `${srcSet.src} ${srcSet.width}w`).join(',');
};

const buildSizesString = (role: RoleType, isMainMedia: boolean): string => {
switch (role) {
case 'inline':
return `(min-width: ${breakpoints.phablet}px) 620px, 100vw`;
case 'halfWidth':
return `(min-width: ${breakpoints.phablet}px) 300px, 50vw`;
case 'thumbnail':
return '140px';
case 'immersive':
return isMainMedia
? '100vw'
: `(min-width: ${breakpoints.wide}px) 1300px, 100vw`;
case 'supporting':
return `(min-width: ${breakpoints.wide}px) 380px, 300px`;
case 'showcase':
return isMainMedia
? `(min-width: ${breakpoints.wide}px) 1020px, (min-width: ${breakpoints.leftCol}px) 940px, (min-width: ${breakpoints.tablet}px) 700px, (min-width: ${breakpoints.phablet}px) 660px, 100vw`
: `(min-width: ${breakpoints.wide}px) 860px, (min-width: ${breakpoints.leftCol}px) 780px, (min-width: ${breakpoints.phablet}px) 620px, 100vw`;
}
};

export const Img = ({
imageSources,
role,
alt,
height,
width,
isMainMedia = false,
isLazy = true,
}: Props) => {
const srcSetForRole = getSrcSetsForRole(role, imageSources);
const src = getFallbackSrc(srcSetForRole);
const sources = buildSourcesString(srcSetForRole);
const sizes = buildSizesString(role, isMainMedia);

return (
<img
itemProp="contentUrl"
alt={alt}
src={src}
srcSet={sources}
sizes={sizes}
height={height}
width={width}
loading={isLazy && !Img.disableLazyLoading ? 'lazy' : undefined}
// https://stackoverflow.com/questions/10844205/html-5-strange-img-always-adds-3px-margin-at-bottom
// why did we add the css `vertical-align: middle;` to the img tag
className={css`
vertical-align: middle;
`}
/>
);
};

// We use disableLazyLoading to decide if we want to turn off lazy loading of images site wide. We use this
// to prevent false negatives on Chromatic snapshots (see /.storybook/config)
Img.disableLazyLoading = false;
50 changes: 0 additions & 50 deletions src/web/components/Picture.tsx

This file was deleted.

100 changes: 15 additions & 85 deletions src/web/components/elements/ImageComponent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { headline } from '@guardian/src-foundations/typography';
import { pillarPalette } from '@root/src/lib/pillars';
import { brandAltBackground, neutral } from '@guardian/src-foundations/palette';

import { Picture, PictureSource } from '@root/src/web/components/Picture';
import { Img } from '@root/src/web/components/Img';
import { Caption } from '@root/src/web/components/Caption';
import { Hide } from '@root/src/web/components/Hide';
import { StarRating } from '@root/src/web/components/StarRating/StarRating';
Expand All @@ -24,78 +24,6 @@ type Props = {
title?: string;
};

const selectScrSetItemForWidth = (
desiredWidth: number,
inlineSrcSets: SrcSetItem[],
): SrcSetItem => {
const sorted = inlineSrcSets.sort((a, b) => b.width - a.width);

return sorted.reduce((best, current) => {
if (current.width < best.width && current.width >= desiredWidth) {
return current;
}

return best;
});
};

const getSrcSetsForWeighting = (
imageSources: ImageSource[],
forWeighting: RoleType,
): SrcSetItem[] =>
imageSources.filter(
({ weighting }) =>
// Use toLowerCase to handle cases where we have halfWidth comparing to halfwidth
weighting.toLowerCase() === forWeighting.toLowerCase(),
)[0].srcSet;

const makePictureSource = (
hidpi: boolean,
minWidth: number,
srcSet: SrcSetItem,
): PictureSource => {
return {
hidpi,
minWidth,
width: srcSet.width,
srcset: `${srcSet.src} ${hidpi ? srcSet.width * 2 : srcSet.width}w`,
};
};

const makeSources = (
imageSources: ImageSource[],
role: RoleType,
): PictureSource[] => {
const inlineSrcSets = getSrcSetsForWeighting(imageSources, role);
const sources: PictureSource[] = [];
inlineSrcSets
.map((item) => item.width)
.forEach((width) => {
sources.push(
makePictureSource(
true,
width,
selectScrSetItemForWidth(width, inlineSrcSets),
),
);
sources.push(
makePictureSource(
false,
width,
selectScrSetItemForWidth(width, inlineSrcSets),
),
);
});

return sources;
};

const getFallback: (imageSources: ImageSource[]) => string = (imageSources) => {
const inlineSrcSets = getSrcSetsForWeighting(imageSources, 'inline');

return selectScrSetItemForWidth(300, inlineSrcSets).src;
};

const starsWrapper = css`
background-color: ${brandAltBackground.primary};

Expand Down Expand Up @@ -291,8 +219,6 @@ export const ImageComponent = ({
starRating,
title,
}: Props) => {
const { imageSources } = element;
const sources = makeSources(imageSources, role);
const shouldLimitWidth =
!isMainMedia &&
(role === 'showcase' || role === 'supporting' || role === 'immersive');
Expand Down Expand Up @@ -332,13 +258,14 @@ export const ImageComponent = ({
}
`}
>
<Picture
sources={sources}
<Img
role={role}
imageSources={element.imageSources}
alt={element.data.alt || ''}
src={getFallback(element.imageSources)}
width={imageWidth}
height={imageHeight}
isLazy={!isMainMedia}
isMainMedia={isMainMedia}
/>
{starRating && <PositionStarRating rating={starRating} />}
{title && (
Expand All @@ -361,13 +288,14 @@ export const ImageComponent = ({
}
`}
>
<Picture
sources={sources}
<Img
role={role}
imageSources={element.imageSources}
alt={element.data.alt || ''}
src={getFallback(element.imageSources)}
width={imageWidth}
height={imageHeight}
isLazy={!isMainMedia}
isMainMedia={isMainMedia}
/>
{starRating && <PositionStarRating rating={starRating} />}
{title && (
Expand All @@ -390,13 +318,14 @@ export const ImageComponent = ({
}
`}
>
<Picture
sources={sources}
<Img
role={role}
imageSources={element.imageSources}
alt={element.data.alt || ''}
src={getFallback(element.imageSources)}
width={imageWidth}
height={imageHeight}
isLazy={!isMainMedia}
isMainMedia={isMainMedia}
/>
{isMainMedia && (
// Below tablet, main media images show an info toggle at the bottom right of
Expand All @@ -419,7 +348,8 @@ export const ImageComponent = ({
}
`}
>
<CaptionToggle />
{/* CaptionToggle contains the input with id #the-checkbox */}
<CaptionToggle />{' '}
<div id="the-caption">
<Caption
display={display}
Expand Down