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

feat: implement responsiveness with srcSet and sizes #159

Merged
merged 29 commits into from
Jul 30, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
560385c
chore(readme): fix typo and update codesandbox examples (#158)
frederickfogerty Jul 5, 2018
c03401a
chore(jest): upgrade jest to support new expect functions
frederickfogerty Jul 22, 2018
0070943
test: use srcset and sizes
frederickfogerty Jul 23, 2018
1bb6808
chore: update build:watch to build es instead of commonjs
frederickfogerty Jul 23, 2018
9c504c9
feat: add constants file and targetWidths from imgix.js
frederickfogerty Jul 23, 2018
ccdbfcf
test: mock window.screen for unit tests
frederickfogerty Jul 23, 2018
d21c5ea
feat: use srcset and sizes
frederickfogerty Jul 23, 2018
0392518
Merge branch 'version-8' into fred/use-srcsets
frederickfogerty Jul 23, 2018
26b0035
test: refactor alt test
frederickfogerty Jul 23, 2018
88eb552
test: refactor ixlib tests to use expectSrcTo
frederickfogerty Jul 23, 2018
6438f15
chore: stop using ...other in props
frederickfogerty Jul 23, 2018
7116c15
chore: rename support.js -> constructUrl.js
frederickfogerty Jul 23, 2018
fac1757
feat: add fallback srcset when using sizes
frederickfogerty Jul 23, 2018
df2db05
feat: update targetWidths with up to date reses, and remove max restr…
frederickfogerty Jul 25, 2018
fa7860c
chore: remove fixed TODO
frederickfogerty Jul 25, 2018
9cf4365
docs: update README for new srcSet/sizes
frederickfogerty Jul 26, 2018
8322693
test: add srcSet test for <source>
frederickfogerty Jul 26, 2018
93a5dfb
feat: use 2x and 3x srcSets for pictures
frederickfogerty Jul 26, 2018
6719d24
docs: update markdown link
frederickfogerty Jul 26, 2018
a7cbe47
docs: add sizes doc
frederickfogerty Jul 26, 2018
7af8394
chore: cleanup unused code
frederickfogerty Jul 26, 2018
c4750f1
chore: update targetWidth
frederickfogerty Jul 26, 2018
ecc0e84
feat: remove type=bg and add reference to issue
frederickfogerty Jul 26, 2018
78c25f0
chore: remove availWidth and availHeight constants as they're not used
frederickfogerty Jul 26, 2018
8108460
chore: update fit logic
frederickfogerty Jul 26, 2018
4e53375
test: remove type=bg tests
frederickfogerty Jul 26, 2018
a6cc892
docs: remove reference to unimplemented aspect ratio behaviour
frederickfogerty Jul 26, 2018
28b074a
docs: add upgrade guide
frederickfogerty Jul 29, 2018
9167095
chore: refactor some code to be more readable
frederickfogerty Jul 29, 2018
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
170 changes: 90 additions & 80 deletions README.md

Large diffs are not rendered by default.

1,091 changes: 446 additions & 645 deletions package-lock.json

Large diffs are not rendered by default.

12 changes: 6 additions & 6 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,17 @@
"build": "npm run clean && npm run build:commonjs && npm run build:es",
"build:commonjs": "cross-env BABEL_ENV=commonjs babel src --out-dir lib --source-maps",
"build:es": "cross-env BABEL_ENV=es babel src --out-dir es --source-maps",
"build:watch": "cross-env BABEL_ENV=commonjs babel src --out-dir lib --watch",
"build:watch": "cross-env BABEL_ENV=es babel src --out-dir es --watch",
"clean": "rimraf lib es",
"prepublish": "npm run build",
"pretty": "prettier --write \"src/**/*.js\"",
"release": "standard-version",
"test:headless": "karma start config/karma/karmaConfigHeadless.js --singleRun",
"test:headless:watch": "karma start config/karma/karmaConfigHeadless.js",
"test:browser": "karma start config/karma/karmaConfigLocal.js --singleRun",
"test:browser:watch": "karma start config/karma/karmaConfigLocal.js",
"test:browser:ci": "karma start config/karma/karmaConfigHeadlessCI.js --singleRun",
"test:browser:browserstack": "karma start config/karma/karmaConfigBrowserStack.js --singleRun",
"test:browser:watch": "karma start config/karma/karmaConfigLocal.js",
"test:browser:ci": "karma start config/karma/karmaConfigHeadlessCI.js --singleRun",
"test:browser:browserstack": "karma start config/karma/karmaConfigBrowserStack.js --singleRun",
"test:browser:all": "npm run test:browser:ci && npm run test:browser:browserstack",
"test:watch": "jest --watch",
"test": "jest && npm run test:browser:all"
Expand Down Expand Up @@ -68,8 +68,8 @@
"cross-env": "^5.1.1",
"enzyme": "^3.3.0",
"enzyme-adapter-react-16": "^1.1.1",
"expect": "^21.2.1",
"jest": "^22.0.0",
"expect": "^23.4.0",
"jest": "^23.4.1",
"jest-extended": "^0.7.2",
"karma": "^2.0.4",
"karma-browserstack-launcher": "^1.3.0",
Expand Down
1 change: 1 addition & 0 deletions src/common.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default as CONSTANTS } from "./constants";
3 changes: 3 additions & 0 deletions src/constants.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
const CONSTANTS = {};

export default CONSTANTS;
11 changes: 5 additions & 6 deletions src/support.js → src/constructUrl.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,10 @@ var PARAM_EXPANSION = Object.freeze({
});

var DEFAULT_OPTIONS = Object.freeze({
auto: "format", // http://www.imgix.com/docs/reference/automatic#param-auto
dpr: 1
auto: "format" // http://www.imgix.com/docs/reference/automatic#param-auto
});

function constructUrl(src, params) {
function constructUrlFromParams(src, params) {
var optionKeys = Object.keys(params);
var fullUrl = optionKeys.reduce(function(uri, key) {
return uri.addQueryParam(key, params[key]);
Expand All @@ -121,7 +120,7 @@ function constructUrl(src, params) {
* @param {Object} longOptions map of image API options, in long or short form per expansion rules
* @return {String} URL of image src transformed by Imgix
*/
function processImage(src, longOptions) {
function constructUrl(src, longOptions) {
if (!src) {
return "";
}
Expand All @@ -143,7 +142,7 @@ function processImage(src, longOptions) {
shortOptions[key] = val;
});

return constructUrl(src, shortOptions);
return constructUrlFromParams(src, shortOptions);
}

module.exports = processImage;
export default constructUrl;
216 changes: 109 additions & 107 deletions src/react-imgix.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,44 +4,20 @@ import ReactDOM from "react-dom";
import React, { Component } from "react";
import PropTypes from "prop-types";

import processImage from "./support.js";

// Best way to include an img with an empty src https://stackoverflow.com/a/5775621/515634 and https://stackoverflow.com/a/19126281/515634
// Using '//:0' doesn't work in IE 11, but using a data-uri works.
const EMPTY_IMAGE_SRC =
"data:image/gif;base64,R0lGODlhAQABAAD/ACwAAAAAAQABAAACADs=";
import targetWidths from "./targetWidths";
import constructUrl from "./constructUrl";

const PACKAGE_VERSION = require("../package.json").version;

const roundToNearest = (size, precision) =>
precision * Math.ceil(size / precision);
const NODE_ENV = process.env.NODE_ENV;
Copy link

Choose a reason for hiding this comment

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

Interesting. How does this end up manifesting when this code is run in-browser?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an extremely common pattern which is used everywhere in React. It is usually defined by webpack plugin or webpack itself.


const isStringNotEmpty = str =>
str && typeof str === "string" && str.length > 0;
const buildKey = idx => `react-imgix-${idx}`;

const validTypes = ["bg", "img", "picture", "source"];

const defaultMap = {
width: "defaultWidth",
height: "defaultHeight"
};

const findSizeForDimension = (dim, props = {}, state = {}) => {
if (props[dim]) {
return props[dim];
} else if (props.fluid && state[dim]) {
return roundToNearest(state[dim], props.precision);
} else if (props[defaultMap[dim]]) {
return props[defaultMap[dim]];
} else {
return 1;
}
};
const validTypes = ["img", "picture", "source"];

export default class ReactImgix extends Component {
static propTypes = {
aggressiveLoad: PropTypes.bool,
auto: PropTypes.array,
children: PropTypes.any,
className: PropTypes.string,
Expand All @@ -51,136 +27,158 @@ export default class ReactImgix extends Component {
entropy: PropTypes.bool,
faces: PropTypes.bool,
fit: PropTypes.string,
fluid: PropTypes.bool,
generateSrcSet: PropTypes.bool,
disableSrcSet: PropTypes.bool,
onMounted: PropTypes.func,
sizes: PropTypes.string,
src: PropTypes.string.isRequired,
type: PropTypes.oneOf(validTypes),
width: PropTypes.number,
height: PropTypes.number,
defaultHeight: PropTypes.number,
defaultWidth: PropTypes.number,
disableLibraryParam: PropTypes.bool
};
static defaultProps = {
aggressiveLoad: false,
auto: ["format"],
entropy: false,
faces: true,
fit: "crop",
fluid: true,
generateSrcSet: true,
disableSrcSet: false,
onMounted: () => {},
precision: 100,
type: "img"
};
state = {
width: null,
height: null,
mounted: false
};

forceLayout = () => {
componentDidMount = () => {
const node = ReactDOM.findDOMNode(this);
this.setState({
width: node.scrollWidth,
height: node.scrollHeight,
mounted: true
});
this.props.onMounted(node);
};

componentDidMount = () => {
this.forceLayout();
};
buildSrcs = () => {
const props = this.props;
const {
width,
height,
entropy,
faces,
auto,
customParams,
disableLibraryParam,
disableSrcSet,
type
} = props;

let crop = false;
if (faces) crop = "faces";
if (entropy) crop = "entropy";
if (props.crop) crop = props.crop;

let fit = false;
if (entropy || faces) fit = "crop";
if (props.fit) fit = props.fit;

const fixedSize = width != null || height != null;

const srcOptions = {
auto,
...customParams,
crop,
fit,
...(disableLibraryParam ? {} : { ixlib: `react-${PACKAGE_VERSION}` }),
...(fixedSize && height ? { height } : {}),
...(fixedSize && width ? { width } : {})
};

_findSizeForDimension = dim =>
findSizeForDimension(dim, this.props, this.state);
const src = constructUrl(this.props.src, srcOptions);

let srcSet;

if (disableSrcSet) {
srcSet = src;
Copy link

Choose a reason for hiding this comment

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

Hmm... the disabledSrcSet option doesn't actually disabled srcset. Is that bad? Maybe there's no good reason not to just set it anyway as we're doing here, but if the option says "don't set it" the user expectation might be that it doesn't get set. I don't feel strongly either way, but it's food for thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll be honest, it isn't very clear, by just reading this section of the code, what the actual behaviour is. I'll explain:

option says "don't set it" the user expectation might be that it doesn't get set

This is the actual behaviour. This value is not automatically applied to rendered element. For an img, if disableSrcSet is set then srcSet is not applied. For a source, if srcSet is set then the srcSet attribute is simply the source, without any px or w generations applied. For source this is desired as srcSet is required in this case.

Does this make more sense? Is there a way we can improve the code?

Copy link

Choose a reason for hiding this comment

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

Yea, I'm on board now 👍

} else {
if (fixedSize || type === "source") {
const dpr2 = constructUrl(this.props.src, { ...srcOptions, dpr: 2 });
const dpr3 = constructUrl(this.props.src, { ...srcOptions, dpr: 3 });
srcSet = `${dpr2} 2x, ${dpr3} 3x`;
} else {
const buildSrcSetPair = targetWidth => {
const url = constructUrl(this.props.src, {
...srcOptions,
width: targetWidth
});
return `${url} ${targetWidth}w`;
};
const addFallbackSrc = srcSet => srcSet.concat(src);
srcSet = addFallbackSrc(targetWidths.map(buildSrcSetPair)).join(", ");
}
}

return {
src,
srcSet
};
};

render() {
const {
aggressiveLoad,
auto,
bg,
children,
component,
customParams,
crop,
entropy,
faces,
fit,
generateSrcSet,
src,
disableSrcSet,
type,
...other
width,
height
} = this.props;
let _src = EMPTY_IMAGE_SRC;
let srcSet = null;
let _component = component;

let width = this._findSizeForDimension("width");
let height = this._findSizeForDimension("height");

let _crop = false;
if (faces) _crop = "faces";
if (entropy) _crop = "entropy";
if (crop) _crop = crop;
// Pre-render checks
if (NODE_ENV !== "production") {
if (
type === "img" &&
width == null &&
height == null &&
this.props.sizes == null &&
!this.props._inPicture
) {
console.warn(
"If width and height are not set, a sizes attribute should be passed."
);
}
}

let _fit = false;
if (entropy) _fit = "crop";
if (fit) _fit = fit;
let _component = component;
const imgProps = this.props.imgProps || {};

let _children = children;

if (this.state.mounted || aggressiveLoad) {
const srcOptions = {
auto: auto,
...customParams,
crop: _crop,
fit: _fit,
width,
height,
...(this.props.disableLibraryParam
? {}
: { ixlib: `react-${PACKAGE_VERSION}` })
};

_src = processImage(src, srcOptions);
const dpr2 = processImage(src, { ...srcOptions, dpr: 2 });
const dpr3 = processImage(src, { ...srcOptions, dpr: 3 });
srcSet = `${dpr2} 2x, ${dpr3} 3x`;
}

let _alt = (this.props.imgProps || {}).alt;
const { src, srcSet } = this.buildSrcs();

let childProps = {
...this.props.imgProps,
sizes: this.props.sizes,
className: this.props.className,
width: other.width <= 1 ? null : other.width,
height: other.height <= 1 ? null : other.height,
alt: this.state.mounted || aggressiveLoad ? _alt : undefined
width: width <= 1 ? null : width,
height: height <= 1 ? null : height,
alt: imgProps.alt
};

switch (type) {
case "bg":
if (!component) {
_component = "div";
}
childProps.style = {
backgroundSize: "cover",
backgroundImage: isStringNotEmpty(_src) ? `url('${_src}')` : null,
...childProps.style
};
// TODO: Remove in v9
throw new Error(
`type='bg' has been removed in this version of react-imgix. If you would like this re-implemented please give this issues a thumbs up: https://github.com/imgix/react-imgix/issues/160`
);
break;
case "img":
if (!component) {
_component = "img";
}

if (generateSrcSet) {
if (!disableSrcSet) {
childProps.srcSet = srcSet;
}
childProps.src = _src;
childProps.src = src;
break;
case "source":
if (!component) {
Expand All @@ -189,14 +187,15 @@ export default class ReactImgix extends Component {

// strip out the "alt" tag from childProps since it's not allowed
delete childProps.alt;
delete childProps.src;

// inside of a <picture> element a <source> element ignores its src
// attribute in favor of srcSet so we set that with either an actual
// srcSet or a single src
if (generateSrcSet) {
childProps.srcSet = `${_src}, ${srcSet}`;
if (disableSrcSet) {
childProps.srcSet = src;
} else {
childProps.srcSet = _src;
childProps.srcSet = `${src}, ${srcSet}`;
}
// for now we'll take media from imgProps which isn't ideal because
// a) this isn't an <img>
Expand All @@ -220,7 +219,10 @@ export default class ReactImgix extends Component {
// make sure all of our children have key set, otherwise we get react warnings
_children =
React.Children.map(children, (child, idx) =>
React.cloneElement(child, { key: buildKey(idx) })
React.cloneElement(child, {
key: buildKey(idx),
_inPicture: true
})
) || [];

// look for an <img> or <ReactImgix type='img'> - at the bare minimum we
Expand Down
Loading