Skip to content
This repository has been archived by the owner on Aug 13, 2023. It is now read-only.

Make width optional #492

Merged
17 commits merged into from
May 9, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
2 changes: 2 additions & 0 deletions packages/components/psammead-image/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
# Psammead Image Changelog

<!-- prettier-ignore -->
| Version | Description |
|---------|-------------|
| 0.3.6 | [PR#492](https://github.com/bbc/psammead/pull/492) Make img width prop optional |
| 0.3.5 | [PR#424](https://github.com/bbc/psammead/pull/424) Add Snyk badge to readme |
| 0.3.4 | [PR#407](https://github.com/bbc/psammead/pull/407) Organise dependencies properly |
| 0.3.3 | [PR#323](https://github.com/bbc/psammead/pull/323) Update storybook badge url |
Expand Down
11 changes: 8 additions & 3 deletions packages/components/psammead-image/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,17 @@ const WrappingContainer = ({ alt, src, height, width }) => (
| Prop | Type | Required | Default | Example |
| :------- | :------------ | :------- | :------ | :----------------------------------------------------------------------------------------------------- |
| `alt` | string | Yes | - | "A picture of a cat" |
| `height` | number/string | Yes | null | 450 |
| `height` | number/string | No | null | 450 |
| `src` | string | Yes | - | "https://bbc.com/300/cat.jpg" |
| `srcset` | string | No | null | "https://bbc.com/300/cat.jpg 300w, https://bbc.com/450/cat.jpg 450w, https://bbc.com/600/cat.jpg 600w" |
| `width` | number/string | Yes | - | 600 |
| `width` | number/string | No | null | 600 |

The `height` and `width` props are optional, in some cases to preserve the image ratio you might specify either `height` or `width` and let the browser scale the image accordingly.

However when not specified the browser will not be able to determine the size of the image, the browser will therefore build the page twice or more depending on the number of images you have. First build is for the browser to display all the text and once the image is downloaded and size determined a second build to wrap the texts around the image.
This conversation was marked as resolved.
Show resolved Hide resolved

Specifying the `width` and `height` allows the browser to reserve space for the image which prevent content moving around while the image is being loaded.

The `height` prop is optional, since in some cases to preserve the image ratio we only want to specify the width and let the browser scale the image accordingly. However, in other cases the height might need to be specified.
The `srcset` prop is optional since some projects might not want to use the srcset attribute on images.

### AmpImg
Expand Down
2 changes: 1 addition & 1 deletion packages/components/psammead-image/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 packages/components/psammead-image/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@bbc/psammead-image",
"version": "0.3.5",
"version": "0.3.6",
"main": "dist/index.js",
"description": "React styled components for an Image",
"repository": {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,21 +1,6 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Image - imported as '{ Img }' should render image with custom dimensions correctly 1`] = `
.c0 {
display: block;
width: 100%;
}

<img
alt="By Elisa Decker, from her series \\"Sidewalk\\""
className="c0"
height={547}
src="https://ichef.bbci.co.uk/news/300/cpsprodpb/164BB/production/_104032319_03270dcc-9dda-4bd4-96a0-db89f6b915ae.jpg"
width={445}
/>
`;

exports[`Image - imported as '{ Img }' should render image with srcset correctly 1`] = `
exports[`Image - Img' should render image correctly without width 1`] = `
.c0 {
display: block;
width: 100%;
Expand All @@ -27,56 +12,10 @@ exports[`Image - imported as '{ Img }' should render image with srcset correctly
height={576}
src="https://ichef.bbci.co.uk/news/300/cpsprodpb/7098/production/_104842882_students.jpg"
srcSet="https://ichef.bbci.co.uk/news/300/cpsprodpb/7098/production/_104842882_students.jpg 300w, https://ichef.bbci.co.uk/news/450/cpsprodpb/7098/production/_104842882_students.jpg 450w, https://ichef.bbci.co.uk/news/600/cpsprodpb/7098/production/_104842882_students.jpg 600w, https://ichef.bbci.co.uk/news/1024/cpsprodpb/7098/production/_104842882_students.jpg 1024w"
width={1024}
/>
`;

exports[`Image - imported as '{ Img }' should render landscape image correctly 1`] = `
.c0 {
display: block;
width: 100%;
}

<img
alt="Student sitting an exam"
className="c0"
height={576}
src="https://ichef.bbci.co.uk/news/300/cpsprodpb/7098/production/_104842882_students.jpg"
width={1024}
/>
`;

exports[`Image - imported as '{ Img }' should render portrait image correctly 1`] = `
.c0 {
display: block;
width: 100%;
}

<img
alt="Map of the UK displaying Syrian refugees and asylum seekers per 10000 population. Ranges from 0 to 17."
className="c0"
height={1280}
src="https://ichef.bbci.co.uk/news/300/cpsprodpb/439A/production/_100960371_syrians_and_asylum_v2-nc.png"
width={1024}
/>
`;

exports[`Image - imported as '{ Img }' should render square image correctly 1`] = `
.c0 {
display: block;
width: 100%;
}

<img
alt="Tracks through the snow"
className="c0"
height={1024}
src="https://ichef.bbci.co.uk/news/300/cpsprodpb/114FE/production/_104801907_79010.jpg"
width={1024}
/>
`;

exports[`Image - imported as default 'Image' should render image with custom dimensions correctly 1`] = `
exports[`Image - Img' should render image with custom dimensions correctly 1`] = `
.c0 {
display: block;
width: 100%;
Expand All @@ -91,7 +30,7 @@ exports[`Image - imported as default 'Image' should render image with custom dim
/>
`;

exports[`Image - imported as default 'Image' should render image with srcset correctly 1`] = `
exports[`Image - Img' should render image with srcset correctly 1`] = `
.c0 {
display: block;
width: 100%;
Expand All @@ -107,7 +46,7 @@ exports[`Image - imported as default 'Image' should render image with srcset cor
/>
`;

exports[`Image - imported as default 'Image' should render landscape image correctly 1`] = `
exports[`Image - Img' should render landscape image correctly 1`] = `
.c0 {
display: block;
width: 100%;
Expand All @@ -122,7 +61,7 @@ exports[`Image - imported as default 'Image' should render landscape image corre
/>
`;

exports[`Image - imported as default 'Image' should render portrait image correctly 1`] = `
exports[`Image - Img' should render portrait image correctly 1`] = `
.c0 {
display: block;
width: 100%;
Expand All @@ -137,7 +76,7 @@ exports[`Image - imported as default 'Image' should render portrait image correc
/>
`;

exports[`Image - imported as default 'Image' should render square image correctly 1`] = `
exports[`Image - Img' should render square image correctly 1`] = `
.c0 {
display: block;
width: 100%;
Expand Down
6 changes: 5 additions & 1 deletion packages/components/psammead-image/src/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ export const Img = ({ alt, src, srcset, height, width }) => {
if (srcset) {
props.srcSet = srcset;
}
if (!width) {
This conversation was marked as resolved.
Show resolved Hide resolved
delete props.width;
}

return <StyledImg {...props} />;
};
Expand All @@ -24,12 +27,13 @@ Img.propTypes = {
src: string.isRequired,
srcset: string,
height: oneOfType([string, number]),
width: oneOfType([string, number]).isRequired,
width: oneOfType([string, number]),
};

Img.defaultProps = {
height: null,
srcset: null,
width: null,
};

export default Img;
8 changes: 2 additions & 6 deletions packages/components/psammead-image/src/index.test.jsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
import Image, { Img } from '.';
import Image from '.';
import snapshotTests from './testHelpers/snapshotTests';

describe("Image - imported as '{ Img }'", () => {
snapshotTests(Img);
});

describe("Image - imported as default 'Image'", () => {
describe("Image - Img'", () => {
This conversation was marked as resolved.
Show resolved Hide resolved
snapshotTests(Image);
});
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,18 @@ const snapshotTests = (Component, additionalProps) => {
width={landscape.width}
/>,
);
if (Component.name === 'Img') {
This conversation was marked as resolved.
Show resolved Hide resolved
shouldMatchSnapshot(
'should render image correctly without width',
<Component
alt={landscape.alt}
attribution={landscape.attribution}
src={landscape.src}
srcset={landscape.srcset}
height={landscape.height}
/>,
);
}
};

export default snapshotTests;
19 changes: 17 additions & 2 deletions packages/components/psammead-image/src/testHelpers/stories.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ const stories = (
includeHeight = false,
additionalProps = {},
styleDecorator = storyFn => storyFn(),
) =>
storiesOf(title, module)
) => {
const imageStories = storiesOf(title, module)
This conversation was marked as resolved.
Show resolved Hide resolved
.addDecorator(styleDecorator)
.add(
'landscape image',
Expand Down Expand Up @@ -69,5 +69,20 @@ const stories = (
),
{ notes },
);
if (Component.name === 'Img') {
This conversation was marked as resolved.
Show resolved Hide resolved
imageStories.add(
'image without width',
() => (
<Component
{...getProps(landscape, includeHeight)}
{...additionalProps}
width={null}
/>
),
{ notes },
);
}
return imageStories;
};

export default stories;