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

Integrate amp img #1153

Merged
merged 12 commits into from
Jan 14, 2019
6 changes: 3 additions & 3 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
"@bbc/psammead-copyright": "^0.2.4",
"@bbc/psammead-figure": "^0.1.1",
"@bbc/psammead-headings": "^0.1.7",
"@bbc/psammead-image": "^0.2.3",
"@bbc/psammead-image": "^0.3.0",
"@bbc/psammead-image-placeholder": "^0.1.5",
"@bbc/psammead-inline-link": "^0.1.3",
"@bbc/psammead-paragraph": "^0.1.3",
Expand Down
406 changes: 369 additions & 37 deletions src/app/containers/Figure/__snapshots__/index.test.jsx.snap

Large diffs are not rendered by default.

49 changes: 39 additions & 10 deletions src/app/containers/Figure/fixtureData.jsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React from 'react';
import FigureContainer from '.';
import { ServiceContext } from '../../contexts/ServiceContext';
import { PlatformContext } from '../../contexts/PlatformContext';
import { blockContainingText } from '../../models/blocks';

const imageAlt = 'Pauline Clayton';
Expand Down Expand Up @@ -69,35 +70,63 @@ const serviceContextStubNews = {
imageCaptionOffscreenText: 'Image caption, ',
};

const generateFixtureData = (caption, copyright) => (
const generateFixtureData = (caption, copyright, platform = 'canonical') => (
<ServiceContext.Provider value={serviceContextStubNews}>
<FigureContainer
alt={imageAlt}
captionBlock={caption}
copyright={copyright}
height={imageHeight}
ratio={imageRatio}
src={imageSrc}
width={imageWidth}
/>
<PlatformContext.Provider value={platform}>
<FigureContainer
alt={imageAlt}
captionBlock={caption ? captionBlock : null}
copyright={copyright ? copyrightText : null}
height={imageHeight}
ratio={imageRatio}
src={imageSrc}
width={imageWidth}
/>
</PlatformContext.Provider>
</ServiceContext.Provider>
);

export const FigureImage = generateFixtureData();

export const FigureAmpImage = generateFixtureData(null, null, 'amp');
dr3 marked this conversation as resolved.
Show resolved Hide resolved

export const FigureImageWithCaption = generateFixtureData(captionBlock);

export const FigureAmpImageWithCaption = generateFixtureData(
captionBlock,
null,
'amp',
);

export const FigureImageWithCopyright = generateFixtureData(
null,
copyrightText,
);

export const FigureAmpImageWithCopyright = generateFixtureData(
null,
copyrightText,
'amp',
);

export const FigureImageWithCopyrightAndCaption = generateFixtureData(
captionBlock,
copyrightText,
);

export const FigureAmpImageWithCopyrightAndCaption = generateFixtureData(
captionBlock,
copyrightText,
'amp',
);

export const FigureImageWithCaptionContainingLink = generateFixtureData(
captionBlockWithLink,
null,
);

export const FigureAmpImageWithCaptionContainingLink = generateFixtureData(
captionBlockWithLink,
null,
'amp',
);
23 changes: 21 additions & 2 deletions src/app/containers/Figure/index.jsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import React from 'react';
import { string, number, objectOf, any } from 'prop-types';
import Figure from '@bbc/psammead-figure';
import Image from '@bbc/psammead-image';
import Image, { AmpImg } from '@bbc/psammead-image';
import ImagePlaceholder from '@bbc/psammead-image-placeholder';
import Copyright from '../Copyright';
import Caption from '../Caption';
import { PlatformContextConsumer } from '../../contexts/PlatformContext';

const renderCopyright = copyright =>
copyright ? <Copyright>{copyright}</Copyright> : null;
Expand All @@ -17,11 +18,27 @@ const FigureContainer = ({
captionBlock,
ratio,
src,
height,
width,
}) => (
<Figure>
<ImagePlaceholder ratio={ratio}>
<Image alt={alt} src={src} width={width} />
<PlatformContextConsumer>
{platform =>
platform === 'amp' ? (
<AmpImg
alt={alt}
attribution={copyright || ''}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm so this means we'll be adding an amp attribution and rendering the copyright below it. How is attribution actually used? Is it meta information or is it read out to screen readers?

Copy link
Author

Choose a reason for hiding this comment

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

I think its just meta information

layout="responsive"
src={src}
height={height}
width={width}
/>
) : (
<Image alt={alt} src={src} width={width} />
)
}
</PlatformContextConsumer>
{renderCopyright(copyright)}
</ImagePlaceholder>
{renderCaption(captionBlock)}
Expand All @@ -34,12 +51,14 @@ FigureContainer.propTypes = {
copyright: string,
ratio: number.isRequired,
src: string.isRequired,
height: number,
width: number.isRequired,
};

FigureContainer.defaultProps = {
copyright: null,
captionBlock: null,
height: null,
};

export default FigureContainer;
20 changes: 20 additions & 0 deletions src/app/containers/Figure/index.stories.jsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
import { storiesOf } from '@storybook/react'; // eslint-disable-line import/no-extraneous-dependencies
import {
FigureImage,
FigureAmpImage,
FigureImageWithCaption,
FigureAmpImageWithCaption,
FigureImageWithCopyright,
FigureAmpImageWithCopyright,
FigureImageWithCopyrightAndCaption,
FigureAmpImageWithCopyrightAndCaption,
FigureImageWithCaptionContainingLink,
FigureAmpImageWithCaptionContainingLink,
} from './fixtureData';
import AmpDecorator from '../../helpers/storybook/ampDecorator';

storiesOf('Figure', module)
.add('with a caption', () => FigureImageWithCaption)
Expand All @@ -19,3 +25,17 @@ storiesOf('Figure', module)
'with a caption containing an inline link',
() => FigureImageWithCaptionContainingLink,
);

storiesOf('Figure - AMP', module)
.addDecorator(AmpDecorator)
.add('with a caption', () => FigureAmpImageWithCaption)
.add('without a caption', () => FigureAmpImage)
.add('with non-BBC copyright', () => FigureAmpImageWithCopyright)
.add(
'with a caption and non-BBC copyright',
() => FigureAmpImageWithCopyrightAndCaption,
)
.add(
'with a caption containing an inline link',
() => FigureAmpImageWithCaptionContainingLink,
);
44 changes: 40 additions & 4 deletions src/app/containers/Figure/index.test.jsx
Original file line number Diff line number Diff line change
@@ -1,26 +1,62 @@
import { shouldMatchSnapshot } from '../../helpers/tests/testHelpers';
import {
FigureImage,
FigureAmpImage,
FigureImageWithCaption,
FigureAmpImageWithCaption,
FigureImageWithCopyright,
FigureAmpImageWithCopyright,
FigureImageWithCopyrightAndCaption,
FigureAmpImageWithCopyrightAndCaption,
FigureImageWithCaptionContainingLink,
FigureAmpImageWithCaptionContainingLink,
} from './fixtureData';

describe('Figure', () => {
shouldMatchSnapshot('should render an image with alt text', FigureImage);

shouldMatchSnapshot('should render copyright text', FigureImageWithCopyright);
shouldMatchSnapshot(
'should render an AMP image with alt text',
FigureAmpImage,
);

shouldMatchSnapshot(
'should render an image with copyright text',
FigureImageWithCopyright,
);

shouldMatchSnapshot('should render caption text', FigureImageWithCaption);
shouldMatchSnapshot(
'should render an AMP image with copyright text',
FigureAmpImageWithCopyright,
);

shouldMatchSnapshot(
'should render an image with caption text',
FigureImageWithCaption,
);

shouldMatchSnapshot(
'should render caption text with inline link',
'should render can AMP image with aption text',
FigureAmpImageWithCaption,
);

shouldMatchSnapshot(
'should render an image with caption text with inline link',
FigureImageWithCaptionContainingLink,
);

shouldMatchSnapshot(
'should render caption and copyright',
'should render an AMP image with caption text with inline link',
FigureAmpImageWithCaptionContainingLink,
);

shouldMatchSnapshot(
'should render an image with caption and copyright',
FigureImageWithCopyrightAndCaption,
);

shouldMatchSnapshot(
'should render an AMP image with caption and copyright',
FigureAmpImageWithCopyrightAndCaption,
);
});
2 changes: 1 addition & 1 deletion src/app/contexts/PlatformContext/index.jsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import React from 'react';
import { node, string, func } from 'prop-types';

const PlatformContext = React.createContext('default');
export const PlatformContext = React.createContext('default');
dr3 marked this conversation as resolved.
Show resolved Hide resolved

export const PlatformContextProvider = ({ children, platform }) => (
<PlatformContext.Provider value={platform}>
Expand Down