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

Add new ImagePlaceholderAmp psammead component #8914

Merged
merged 35 commits into from
Apr 7, 2021
Merged
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
7f78599
imported amp image placeholder from psammead
HarveyPeachey Feb 24, 2021
43b6027
updated fixture data with img with no src
HarveyPeachey Feb 24, 2021
8f8afb6
added storybook for new amp img placeholder functionality
HarveyPeachey Feb 24, 2021
fd39084
added unit test for failed amp image load
HarveyPeachey Feb 24, 2021
2b6d5c7
updated snapshots
HarveyPeachey Feb 24, 2021
2bc12a0
adjusts storypromo unit test
HarveyPeachey Feb 24, 2021
00ba860
bumped psammead-image-placeholder
HarveyPeachey Feb 24, 2021
00125f8
re-added srcset prop
HarveyPeachey Feb 24, 2021
90e7a89
Merge branch 'latest' into amp-img-placeholder
HarveyPeachey Feb 24, 2021
18aef55
Merge branch 'latest' into amp-img-placeholder
HarveyPeachey Mar 3, 2021
a68ad79
updated package.json
HarveyPeachey Mar 3, 2021
0b94d0a
updated yarn.lock
HarveyPeachey Mar 3, 2021
ed7e2b9
Merge branch 'latest' into amp-img-placeholder
HarveyPeachey Mar 3, 2021
c91d6c2
updated package.json and yarn.lock
HarveyPeachey Mar 3, 2021
86d5fa0
Merge branch 'amp-img-placeholder' of https://github.com/bbc/simorgh …
HarveyPeachey Mar 3, 2021
a4d04b4
updated unit test snapshots
HarveyPeachey Mar 3, 2021
8b1dd1b
updated integration test snapshots
HarveyPeachey Mar 3, 2021
bcba317
amended canonical style logic
HarveyPeachey Mar 3, 2021
76f9a35
adjusted e2e tests to reflect placeholder logic changes
HarveyPeachey Mar 3, 2021
a256379
Merge branch 'latest' into amp-img-placeholder
HarveyPeachey Mar 3, 2021
15f2eed
Merge branch 'latest' of https://github.com/bbc/simorgh into amp-img-…
simonsinclair Mar 16, 2021
ed2dd77
Merge branch 'latest' of https://github.com/bbc/simorgh into amp-img-…
simonsinclair Mar 17, 2021
ef5d267
Merge branch 'latest' of https://github.com/bbc/simorgh into amp-img-…
simonsinclair Mar 17, 2021
638aa6e
Renew package files
simonsinclair Mar 17, 2021
185af88
Merge branch 'latest' into amp-img-placeholder
HarveyPeachey Mar 24, 2021
6094ddd
adds small fix for copyright message
HarveyPeachey Mar 24, 2021
d387eeb
updated snapshots
HarveyPeachey Mar 24, 2021
c4fc013
Merge branch 'latest' into amp-img-placeholder
HarveyPeachey Mar 24, 2021
448206b
Merge branch 'latest' into amp-img-placeholder
HarveyPeachey Mar 24, 2021
7e667e2
Merge branch 'latest' into amp-img-placeholder
HarveyPeachey Mar 24, 2021
d68fb49
Merge branch 'latest' into amp-img-placeholder
HarveyPeachey Mar 31, 2021
8be721f
Merge branch 'latest' into amp-img-placeholder
HarveyPeachey Mar 31, 2021
871453d
Merge branch 'latest' into amp-img-placeholder
HarveyPeachey Mar 31, 2021
154f959
Merge branch 'latest' into amp-img-placeholder
paruchurisilpa Apr 6, 2021
c140e43
Merge branch 'latest' into amp-img-placeholder
paruchurisilpa Apr 6, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions cypress/integration/pages/articles/tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,6 @@ export const testsThatFollowSmokeTestConfig = ({
});

if (serviceHasFigure(service)) {
it('should have a placeholder image', () => {
cy.get('figure div div div').eq(0).should('be.visible');
});

if (serviceHasCaption(service)) {
it('should have a visible image with a caption, and also not be lazyloaded', () => {
cy.get('figure')
Expand Down
13 changes: 9 additions & 4 deletions cypress/integration/pages/articles/testsForAMPOnly.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,21 @@ export const testsThatFollowSmokeTestConfigForAMPOnly = ({
variant,
}) =>
describe(`Running testsForAMPOnly for ${service} ${pageType}`, () => {
it('should contain an amp-img', () => {
if (serviceHasFigure(service)) {
if (serviceHasFigure(service)) {
it('should contain an amp-img', () => {
cy.get('figure')
.eq(0)
.should('be.visible')
.within(() => {
cy.get('amp-img').should('be.visible');
});
}
});
});

it('should have a placeholder and fallback for amp-image', () => {
cy.get('div[placeholder]').eq(0).should('be.hidden');
cy.get('div[fallback]').eq(0).should('be.hidden');
});
}

describe('Media Player: AMP', () => {
it('should render a placeholder image', () => {
Expand Down
9 changes: 9 additions & 0 deletions cypress/integration/pages/articles/testsForCanonicalOnly.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ import { getBlockData, getBlockByType, getVideoEmbedUrl } from './helpers';
// TODO: Remove after https://github.com/bbc/simorgh/issues/2959
const serviceHasCaption = service => service === 'news';

const serviceHasFigure = service =>
['arabic', 'news', 'pashto', 'persian', 'urdu'].includes(service);

// For testing important features that differ between services, e.g. Timestamps.
// We recommend using inline conditional logic to limit tests to services which differ.
export const testsThatAlwaysRunForCanonicalOnly = ({ service, pageType }) => {
Expand All @@ -32,6 +35,12 @@ export const testsThatFollowSmokeTestConfigForCanonicalOnly = ({
});
}

if (serviceHasFigure(service)) {
it('should have a placeholder image', () => {
cy.get('figure div div div').eq(0).should('be.visible');
});
}

if (serviceHasCaption(service)) {
describe('Image with placeholder', () => {
it('should have a visible image that is not lazyloaded', () => {
Expand Down
405 changes: 270 additions & 135 deletions src/app/containers/ArticleFigure/__snapshots__/index.test.jsx.snap

Large diffs are not rendered by default.

Large diffs are not rendered by default.

28 changes: 19 additions & 9 deletions src/app/containers/ImageWithPlaceholder/fixtureData.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ const serviceContextStubNews = {
imageCaptionOffscreenText: 'Image caption, ',
};

const imageUrl =
'https://ichef.bbci.co.uk/news/640/cpsprodpb/E7DB/production/_101655395_paulineclayton.jpg';

const WrappedImageWithPlaceholder = ({ isAmp, ...otherProps }) => (
<ServiceContext.Provider value={serviceContextStubNews}>
<RequestContextProvider
Expand Down Expand Up @@ -58,7 +61,7 @@ WrappedImageWithPlaceholder.defaultProps = {
width: null,
};

const baseFixture = {
const baseFixture = (src = imageUrl) => ({
alt: 'Pauline Clayton',
children: null,
copyright: 'Getty Images',
Expand All @@ -67,25 +70,32 @@ const baseFixture = {
lazyLoad: false,
isAmp: false,
ratio: 56.25,
src:
'https://ichef.bbci.co.uk/news/640/cpsprodpb/E7DB/production/_101655395_paulineclayton.jpg',
srcset:
'https://ichef.bbci.co.uk/news/640/cpsprodpb/E7DB/production/_101655395_paulineclayton.jpg 640w',
src,
srcset: `${src} 640w`,
width: 640,
};
});

// eslint-disable-next-line react/prop-types
export const ImageWithPlaceholder = ({ preload = false }) => {
const props = {
...baseFixture,
...baseFixture(),
preload,
};
return <WrappedImageWithPlaceholder {...props} />;
};

export const AmpImageWithPlaceholder = () => {
const props = {
...baseFixture,
...baseFixture(),
isAmp: true,
};

return <WrappedImageWithPlaceholder {...props} />;
};

export const AmpImageWithPlaceholderFallback = () => {
const props = {
...baseFixture('foo'),
isAmp: true,
};

Expand All @@ -95,7 +105,7 @@ export const AmpImageWithPlaceholder = () => {
// eslint-disable-next-line react/prop-types
export const LazyLoadImageWithPlaceholder = ({ fallback, lazyLoad = true }) => {
const props = {
...baseFixture,
...baseFixture(),
fallback,
lazyLoad,
};
Expand Down
36 changes: 24 additions & 12 deletions src/app/containers/ImageWithPlaceholder/index.jsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import React, { useContext, useState } from 'react';
import { string, number, bool, node } from 'prop-types';
import LazyLoad from 'react-lazyload';
import ImagePlaceholder from '@bbc/psammead-image-placeholder';
import styled from '@emotion/styled';
import ImagePlaceholder, {
ImagePlaceholderAmp,
} from '@bbc/psammead-image-placeholder';
import Image, { AmpImg } from '@bbc/psammead-image';
import { Helmet } from 'react-helmet';
import { RequestContext } from '#contexts/RequestContext';
Expand Down Expand Up @@ -42,6 +45,10 @@ const ImageWithPlaceholder = ({
<Image onLoad={() => setIsLoaded(true)} {...imageProps} />
);

const AmpImgWrapper = styled.div`
position: relative;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this css needed for?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raise psammead PR as discussed to move this into psammead after merge

`;

const shouldPreload = !isAmp && preload;

return (
Expand All @@ -57,11 +64,8 @@ const ImageWithPlaceholder = ({
/>
</Helmet>
)}
<ImagePlaceholder
style={isLoaded ? { background: 'none' } : null}
ratio={ratio}
>
{isAmp ? (
{isAmp ? (
<AmpImgWrapper>
<AmpImg
alt={alt}
attribution={copyright || ''}
Expand All @@ -70,12 +74,20 @@ const ImageWithPlaceholder = ({
srcset={srcset}
height={height}
width={width}
/>
) : (
renderImage(imageToRender, lazyLoad, fallback)
)}
{children}
</ImagePlaceholder>
>
<ImagePlaceholderAmp />
</AmpImg>
{children}
</AmpImgWrapper>
) : (
<ImagePlaceholder
style={isLoaded ? { background: 'none' } : null}
ratio={ratio}
>
{renderImage(imageToRender, lazyLoad, fallback)}
{children}
</ImagePlaceholder>
)}
</>
);
};
Expand Down
4 changes: 3 additions & 1 deletion src/app/containers/ImageWithPlaceholder/index.stories.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { storiesOf } from '@storybook/react';
import {
ImageWithPlaceholder,
AmpImageWithPlaceholder,
AmpImageWithPlaceholderFallback,
LazyLoadImageWithPlaceholder,
} from './fixtureData';
import AmpDecorator from '../../../../.storybook/helpers/ampDecorator';
Expand All @@ -15,4 +16,5 @@ storiesOf('Containers/Image with Placeholder/Canonical', module)
storiesOf('Containers/Image with Placeholder/AMP', module)
.addParameters({ chromatic: { disable: true } })
.addDecorator(AmpDecorator)
.add('default', () => <AmpImageWithPlaceholder />);
.add('default', () => <AmpImageWithPlaceholder />)
.add('with an invalid img src', () => <AmpImageWithPlaceholderFallback />);
6 changes: 6 additions & 0 deletions src/app/containers/ImageWithPlaceholder/index.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { shouldMatchSnapshot } from '@bbc/psammead-test-helpers';
import {
ImageWithPlaceholder,
AmpImageWithPlaceholder,
AmpImageWithPlaceholderFallback,
LazyLoadImageWithPlaceholder,
} from './fixtureData';

Expand Down Expand Up @@ -65,4 +66,9 @@ describe('ImageWithPlaceholder', () => {
'should render an AMP image',
<AmpImageWithPlaceholder />,
);

shouldMatchSnapshot(
'should render a fallback when AMP image fails to load',
<AmpImageWithPlaceholderFallback />,
);
});
Loading