feat: implement responsiveness with srcSet and sizes #159

frederickfogerty · 2018-07-26T20:24:52Z

BREAKING CHANGE:

srcSet behaviour has changed to use sizes + srcSets
type=bg has been removed
the following props have been removed: aggressiveLoad, component, fluid, precision, defaultHeight, defaultWidth
generateSrcSet has been changed to disableSrcSet

Description

This PR fundamentally changes the approach taken to responsiveness. Previously, the image (or component) was rendered with an empty image, and the size of the resulting DOM element was measured. An image was loaded with these dimensions, ensuring that the image loaded was only as large as needed. Now, this component achieves the same behaviour by leveraging new native browser features, srcSet and sizes. This allows the browser to load an appropriate size based on the expected image size, which is calculated based on sizes. This has an added benefit that images can load much quicker as the browser can load them as soon as page layout has completed, or sooner (e.g. if sizes is 100vw). Overall this change has made both the usage of the library and the internals simpler and easier to understand.

The significant amount of breaking changes makes sense due to a lot of the props no longer making sense. Background mode has also been removed as it was incompatible with the new method of responsiveness, and after discussion with @jayeb it seemed that it wouldn't be missed (based on a similar change that imgix.js made).

Picture mode still doesn't work amazingly, as there is a mis-match between what a developer would expect to set as props and what they actually have to set to make picture work as they want. This is not a problem with our library per se, but with picture pecularities. For example, if the fallback img in a <picture> has a width attribute set, all the other sources will also be locked to that size, even if they specify their own widths. Maybe this is common knowledge but it seems peculiar to me. I expect to address this and make it clearer for developers in a future PR.

This PR closes #153. This PR closes #115. This PR closes #71

Checklist

Please use the checklist that is most closely related to your PR (you only need to use one checklist, and you can skip items that aren't applicable or don't make sense):

New Feature

If this is a big feature with breaking changes, consider [opening an issue][issues] to discuss first. This is completely up to you, but please keep in mind that your PR might not be accepted.
Run unit tests to ensure all existing tests are still passing
Add new passing unit tests to cover the code introduced by your PR
Update the readme
Add some steps so we can test your cool new feature!
The PR title is in the conventional commit format: e.g. feat(<area>): added new way to do this cool thing #issue-number
Add your info to the contributors array in package.json!

Steps to Test

Observe changes to unit tests.

It may also be worth to set up a test project and use npm link to check default image rendering and picture rendering.

* chore(readme): fix typo in readme * chore(readme): edit codesandbox iframes to be links instead

- generateSrcSet -> disableSrcSet - remove aggressiveLoad - remove precision - add sizes top-level prop

- generateSrcSet -> disableSrcSet - remove - remove precision - add sizes top-level prop - remove empty img - remove defaultWidth and defaultHeight - remove fluid prop - remove auto-sizing behaviour in favour of srcsets - add check to ensure sizes is passed when in image mode and no width and heights have been passed

* version-8: fix: warn the user when no <img> passed as a child to <picture> fixes #90 (#151) # Conflicts: # src/react-imgix.js # test/unit/react-imgix.test.js

…iction

jayeb

A couple of small changes, but by and large this looks great.

The biggest issue I have is with the availWidth and availHeight constants--are they even necessary at this point? We already decided that they shouldn't be used to cap the list of generated widths at a specific size, so I'm not sure they have any use anymore.

jayeb · 2018-07-26T22:13:14Z

README.md


-For server rendering, `aggressiveLoad` should be used. This component renders nothing on the first render as it tries to work out what the size of the container it will be rendering in, and only loads an image at the resolution required. For server rendering this will mean no image will be rendered.
+Since imgix can generate as many derivative resolutions as needed, react-imgix calculates them programmatically, using the dimensions you specify (note that the w and h params scale appropriately to maintain the correct aspect ratio). All of this information has been placed into the srcset and sizes attributes.


Maybe I'm missing something, but what's up with the bit about w and h scaling properly? This isn't exemplified in the above code.

Whoops, I was planning to do this but didn't get around to it. I might pull this out and put it in a separate issue

Removed from PR and added to #161

jayeb · 2018-07-26T22:21:36Z

src/constants.js

@@ -0,0 +1,6 @@
+const CONSTANTS = {
+  availWidth: window.screen.availWidth,


I can't find good data about browser support for this. Have we verified that window.screen is available on all browsers we support?

I realise now this isn't used anymore anyway so I'll just remove it.

jayeb · 2018-07-26T22:24:38Z

src/react-imgix.js

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


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

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

jayeb · 2018-07-26T22:29:46Z

src/react-imgix.js

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


I think this should be if (entropy || faces)?

jayeb · 2018-07-26T22:37:07Z

src/react-imgix.js

+    let srcSet;
+
+    if (disableSrcSet) {
+      srcSet = src;


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.

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?

Yea, I'm on board now 👍

jayeb · 2018-07-26T22:40:10Z

src/react-imgix.js

+        const addFallbackSrc = srcSet => srcSet.concat(`, ${src}`);
+        const _srcSet = targetWidths.map(buildSrcSetPair).join(", ");
+        const _srcSetWithFallback = addFallbackSrc(_srcSet);
+        srcSet = _srcSetWithFallback;


This seems like a really convoluted way of doing this. How about just doing this?

srcSet = targetWidths.map(buildSrcSetPair).concat([src]).join(", ");

My aim with _srcSetWithFallback was to make clear what the added src was for. Would this work?

const addFallbackSrc = srcSet => srcSet.concat(src); const _srcSet = targetWidths.map(buildSrcSetPair); const _srcSetWithFallback = addFallbackSrc(_srcSet); srcSet = _srcSetWithFallback.join(', ');

or even

const addFallbackSrc = srcSet => srcSet.concat(src); srcSet = addFallbackSrc(targetWidths.map(buildSrcSetPair)).join(', ');

I think either one of those is an improvement over the code as it's currently written, yea.

jayeb · 2018-07-26T22:51:03Z

src/targetWidths.js

+  });
+}
+
+export default targetWidths();


This is an excellent update to the crusty version found in imgix.js, thank you. At some point in the near future I'd love to pull this out into its own repo with a JSON distributable a la https://github.com/imgix/imgix-url-params.

Thank you. That sounds like a great idea.

jayeb · 2018-07-26T22:54:49Z

src/targetWidths.js

+
+import CONSTANTS from "./constants";
+
+const _screen = {


Is this constant even used?

No, I forgot to remove it when I did the update.

frederickfogerty · 2018-07-26T23:36:03Z

Thanks for the review @jayeb. You're dead right with the constants - I've removed them.

frederickfogerty · 2018-07-29T21:19:35Z

Okay @jayeb I think I've covered everything discussed here - are you able to give this a quick review?

* version-8: docs: add link to fluid example and fix some links docs: add example about fitting to the size of the container docs: add ref information feat: refactor picture and source behaviour into different components (#163) feat: reduce props API surface area (#162) feat: implement responsiveness with srcSet and sizes (#159) fix: warn the user when no <img> passed as a child to <picture> fixes #90 (#151) # Conflicts: # README.md

frederickfogerty added 19 commits July 6, 2018 08:36

chore(readme): fix typo and update codesandbox examples (#158)

560385c

* chore(readme): fix typo in readme * chore(readme): edit codesandbox iframes to be links instead

chore(jest): upgrade jest to support new expect functions

c03401a

test: use srcset and sizes

0070943

- generateSrcSet -> disableSrcSet - remove aggressiveLoad - remove precision - add sizes top-level prop

chore: update build:watch to build es instead of commonjs

1bb6808

feat: add constants file and targetWidths from imgix.js

9c504c9

test: mock window.screen for unit tests

ccdbfcf

Merge branch 'version-8' into fred/use-srcsets

0392518

* version-8: fix: warn the user when no <img> passed as a child to <picture> fixes #90 (#151) # Conflicts: # src/react-imgix.js # test/unit/react-imgix.test.js

test: refactor alt test

26b0035

test: refactor ixlib tests to use expectSrcTo

88eb552

chore: stop using ...other in props

6438f15

chore: rename support.js -> constructUrl.js

7116c15

feat: add fallback srcset when using sizes

fac1757

feat: update targetWidths with up to date reses, and remove max restr…

df2db05

…iction

chore: remove fixed TODO

fa7860c

docs: update README for new srcSet/sizes

9cf4365

test: add srcSet test for <source>

8322693

feat: use 2x and 3x srcSets for pictures

93a5dfb

docs: update markdown link

6719d24

frederickfogerty requested review from jayeb and modosc July 26, 2018 20:24

frederickfogerty added 4 commits July 27, 2018 08:30

docs: add sizes doc

a7cbe47

chore: cleanup unused code

7af8394

chore: update targetWidth

c4750f1

feat: remove type=bg and add reference to issue

ecc0e84

jayeb suggested changes Jul 26, 2018

View reviewed changes

frederickfogerty added 3 commits July 27, 2018 11:18

chore: remove availWidth and availHeight constants as they're not used

78c25f0

chore: update fit logic

8108460

test: remove type=bg tests

4e53375

docs: remove reference to unimplemented aspect ratio behaviour

a6cc892

frederickfogerty mentioned this pull request Jul 26, 2018

v8 Breaking Changes Discussion #144

Closed

frederickfogerty added 2 commits July 30, 2018 09:16

docs: add upgrade guide

28b074a

chore: refactor some code to be more readable

9167095

jayeb approved these changes Jul 30, 2018

View reviewed changes

frederickfogerty merged commit fa68df6 into version-8 Jul 30, 2018

frederickfogerty deleted the fred/use-srcsets branch July 30, 2018 19:48

jayeb mentioned this pull request Jul 30, 2018

feat: reduce props API surface area #162

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement responsiveness with srcSet and sizes #159

feat: implement responsiveness with srcSet and sizes #159

frederickfogerty commented Jul 26, 2018 •

edited

Loading

jayeb left a comment

jayeb Jul 26, 2018

frederickfogerty Jul 26, 2018

frederickfogerty Jul 26, 2018

jayeb Jul 26, 2018

frederickfogerty Jul 26, 2018

jayeb Jul 26, 2018

frederickfogerty Jul 26, 2018

jayeb Jul 26, 2018

frederickfogerty Jul 26, 2018

jayeb Jul 26, 2018

frederickfogerty Jul 26, 2018

jayeb Jul 27, 2018

jayeb Jul 26, 2018

frederickfogerty Jul 26, 2018 •

edited

Loading

jayeb Jul 27, 2018

jayeb Jul 26, 2018

frederickfogerty Jul 26, 2018

jayeb Jul 26, 2018

frederickfogerty Jul 26, 2018

frederickfogerty commented Jul 26, 2018

frederickfogerty commented Jul 29, 2018


		For server rendering, `aggressiveLoad` should be used. This component renders nothing on the first render as it tries to work out what the size of the container it will be rendering in, and only loads an image at the resolution required. For server rendering this will mean no image will be rendered.
		Since imgix can generate as many derivative resolutions as needed, react-imgix calculates them programmatically, using the dimensions you specify (note that the w and h params scale appropriately to maintain the correct aspect ratio). All of this information has been placed into the srcset and sizes attributes.

		@@ -0,0 +1,6 @@
		const CONSTANTS = {
		availWidth: window.screen.availWidth,

feat: implement responsiveness with srcSet and sizes #159

feat: implement responsiveness with srcSet and sizes #159

Conversation

frederickfogerty commented Jul 26, 2018 • edited Loading

Description

Checklist

New Feature

Steps to Test

jayeb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederickfogerty Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederickfogerty commented Jul 26, 2018

frederickfogerty commented Jul 29, 2018

frederickfogerty commented Jul 26, 2018 •

edited

Loading

frederickfogerty Jul 26, 2018 •

edited

Loading