feat: add container to render image as background behind children [WIP] #236

frederickfogerty · 2018-12-04T12:09:41Z

Description

This PR adds a new React component which enables an image to be rendered as a background behind children components.

This used to exist in this library before a previous update removed it. Because of the community support in #160, it has been re-implemented.

I've opened this PR early to gain feedback.

TODO

Discussion Points

Should width and height be top-level props (or just props in htmlAttributes and imgProps)? This was available in the old implementation, so it'd be a +1 to be compatible with that. This is at odds with the strategy taken by this library currently which is to try limit the number of top-level props.
Are there any tests I should add (in addition to the ones I have already added/stubbed in)?
Should there be an onMounted prop? We have a onLoad for our img tag, but this only makes sense because there is an equivalent attribute for a native img tag. I'm thinking we shouldn't.
Should we scale up the background image by the current dpr of the screen?
Shall the width of the background image be snapped to the nearest width available from our srcset widths? This would add the benefits that we get from that feature to this feature, but it could also make the prop configuration confusing. For example, if the user supplies a height, do we render an image with that height and calculate a width for that height, or do we change the height so it will match a srcset-width?

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

Review changes to unit tests.

* master: chore(release): 8.4.0 feat: use exponential increase for srcset widths (#224) chore(deps): update dependency webpack to v4.26.1 (#229) chore: update travis node version to 10.x (#228) feat: expose url builder api (#225) chore(deps): update dependency webpack to v4.26.0 (#226)

jayeb · 2018-12-05T19:01:00Z

Should width and height be top-level props (or just props in htmlAttributes and imgProps)? This was available in the old implementation, so it'd be a +1 to be compatible with that. This is at odds with the strategy taken by this library currently which is to try limit the number of top-level props.

As much as compatibility with the old version seems like a virtue here, I value symmetry and predictability in the interfaces for this library a great deal more. I'd rather we switch to using "nested" props for this, just like we now use nested props for everything else.

Are there any tests I should add (in addition to the ones I have already added/stubbed in)?

I think what you've got looks fine, but to be honest I'm not the best person to ask. My brain doesn't usually work in a tests-first fashion--I have to see the code and understand everything it's doing before I can think about what cases need to be covered by tests.

Should there be an onMounted prop? We have a onLoad for our img tag, but this only makes sense because there is an equivalent attribute for a native img tag. I'm thinking we shouldn't.

Let's skip it, and it can be added in the future if this is something lots of users would find useful.

Should we scale up the background image by the current dpr of the screen?

This sounds like a no-brainer "yes" to me, so I'm thinking maybe there's something I'm not considering here. Is there a reason we wouldn't want to do this?

Shall the width of the background image be snapped to the nearest width available from our srcset widths? This would add the benefits that we get from that feature to this feature, but it could also make the prop configuration confusing. For example, if the user supplies a height, do we render an image with that height and calculate a width for that height, or do we change the height so it will match a srcset-width?

I think snapping to our srcset widths is a great idea. The height question is interesting, though. I don't like the idea of trying to shoehorn someone's specified height into a set of widths--what if we just generated a "height srcset" using the same algorithm as we use for generating widths and pick the correct image from there instead?

frederickfogerty · 2018-12-06T09:14:25Z

I'm on board with all of these responses.

Should we scale up the background image by the current dpr of the screen?

This sounds like a no-brainer "yes" to me, so I'm thinking maybe there's something I'm not considering here. Is there a reason we wouldn't want to do this?

It's just something that wasn't implemented in the previous version, and will involve some more CSS (background-size). I don't know if it will cause any problems, but I just wanted to make sure we were keen to do this before I dived into it.

Shall the width of the background image be snapped to the nearest width available from our srcset widths? This would add the benefits that we get from that feature to this feature, but it could also make the prop configuration confusing. For example, if the user supplies a height, do we render an image with that height and calculate a width for that height, or do we change the height so it will match a srcset-width?

I think snapping to our srcset widths is a great idea. The height question is interesting, though. I don't like the idea of trying to shoehorn someone's specified height into a set of widths--what if we just generated a "height srcset" using the same algorithm as we use for generating widths and pick the correct image from there instead?

Okay, I'll implement the snapping to widths.

Can you elaborate more about what generating a height srcset when the user has passed a height would mean?

The best strategy I can think of right now is: if a user passes in a width or a height, just calculate the other dimension given that dimension, and don't do any "magic". I think it would be confusing for a user, say, if we requested a different height than the height they specified.

frederickfogerty · 2018-12-06T14:08:13Z

This is close now!

frederickfogerty · 2018-12-07T16:49:49Z

@jayeb although the tests are failing #thanksIE I'm happy with the implementation, and would love a review

jayeb

A couple of changes, but this is definitely a step in the right direction. I assume react-imgix-old.js is only in here to make dev easier, so I didn't review it.

Can you pull the .js => .jsx conversion of the test file into a separate PR, or are there too many changes in there to cleanly separate?

jayeb · 2018-12-07T18:41:31Z

README.md

@@ -295,7 +299,44 @@ This works for Source and Picture elements as well.

 #### Background mode

-This feature has been removed from react-imgix when `sizes` and `srcset` was implemented. It was decided that it was too hard to implement this feature consistently. If you would still like to use this feature, please give this issue a thumbs up: [https://github.com/imgix/react-imgix/issues/160](https://github.com/imgix/react-imgix/issues/160) If we get enough requests for this, we will re-implement it.
+Images can be rendered as the background of a container around some children by using `<Background />`. The component will measure the natural size of the container as determined by the CSS on the page, and will render an optimal image for those dimensions.


The "around some children" part of this sentence is very confusing to me. Is there a way to convey this better?

Maybe it could use "behind" instead? Here are some options:

Images can be rendered as a background behind children by using <Background />.

<Background /> allows an image to be the background behind its children.

I like the first one best.

It is done.

jayeb · 2018-12-07T18:44:27Z

README.md

+_For example_:
+
+```js
+<Imgix imgixParams={{ mask: "ellipse" }} />


Should this example use <Background instead?

jayeb · 2018-12-07T18:49:40Z

src/react-imgix-bg.jsx

+const noop = () => {};
+
+const findNearestWidth = actualWidth =>
+  targetWidths.reduce((currentCandidate, candidateWidth) => {


I like the reduce solution here, but isn't it pretty inefficient? It's gonna run through the entire list of widths every time, even if it finds the correct value after examining the very first candidate. I think a do-while would be better here.

Yeah it's inefficient. I'll investigate efficient ways to find the nearest value in a sorted list.

Replaced with a binary search which has O(log n) search complexity

Frankly, this seems like overkill too! We know targetWidths is sorted already, so can't we just do a simple while loop that checks each width starting from the lowest one and exits as soon as it finds one that's big enough?

jayeb · 2018-12-07T18:51:51Z

src/react-imgix-bg.jsx

+  const { w: forcedWidth, h: forcedHeight } = imgixParams;
+  const hasDOMDimensions = contentRect.bounds.top != null;
+  const htmlAttributes = props.htmlAttributes || {};
+  const dpr = imgixParams.dpr || window.devicePixelRatio || 1;


We should round window.devicePixelRatio to 2 decimal places here, since anything beyond 2 decimal places is disregarded by the painter and just hurts the cache.

jayeb · 2018-12-07T19:49:26Z

src/react-imgix-bg.jsx

+      width,
+      height,
+      fit: "crop",
+      backgroundSize: "cover",


Ah, isn't this accidentally adding &backgroundSize=cover to all of our imgix URLs?

Whoops, don't know how this slipped in here.

* master: chore(deps): update dependency enzyme to v3.8.0 (#241) chore(deps): update dependency enzyme-adapter-react-16 to v1.7.1 (#240) chore(deps): update dependency sinon to v7.2.0 (#239) chore: rename files containing JSX to .jsx (#238) # Conflicts: # test/integration/react-imgix.test.jsx

…-imgix into fred/background-mode * 'fred/background-mode' of https://github.com/imgix/react-imgix: Sync with Prettier

src/react-imgix-bg.jsx

adamraider · 2018-12-20T15:53:32Z

src/react-imgix-bg.jsx

+
+const findNearestWidth = actualWidth => findClosest(actualWidth, targetWidths);
+
+const toFixed = (dp, value) => +value.toFixed(2);


dp is not used?

adamraider · 2018-12-20T16:10:16Z

src/react-imgix-bg.jsx

+  const { w: forcedWidth, h: forcedHeight } = imgixParams;
+  const hasDOMDimensions = contentRect.bounds.top != null;
+  const htmlAttributes = props.htmlAttributes || {};
+  const dpr = toFixed(2, imgixParams.dpr || window.devicePixelRatio || 1);


To support SSR:

Suggested change

const dpr = toFixed(2, imgixParams.dpr || window.devicePixelRatio || 1);

const dpr = toFixed(2, imgixParams.dpr || global.devicePixelRatio || 1);

Co-Authored-By: frederickfogerty <frederick.fogerty@gmail.com>

frederickfogerty · 2018-12-21T10:06:21Z

Thanks for the very helpful review @adamraider. The tests pass locally but there are just a few issues remaining with IE 11 that I need to investigate before merging this.

…-imgix into fred/background-mode * 'fred/background-mode' of https://github.com/imgix/react-imgix: Sync with Prettier

frederickfogerty added 3 commits November 20, 2018 21:46

WIP

c92dcc0

chore: wip

e80a756

frederickfogerty requested a review from jayeb December 4, 2018 12:09

frederickfogerty added 3 commits December 4, 2018 13:19

Merge branch 'master' into fred/background-mode

31931a5

test: implement more background tests

0b32307

feat: add html attribute override before load

0545553

frederickfogerty mentioned this pull request Dec 6, 2018

Add background mode back #160

Closed

2 tasks

frederickfogerty and others added 8 commits December 7, 2018 14:42

test: update test cases

94b2131

feat: refactor implementation

2cc11bc

Sync with Prettier

30a50f7

test: add test for html overrides after element loaded

767fe92

Sync with Prettier

5db73c3

Merge branch 'master' into fred/background-mode

d3e1492

docs: add information about <Background />

bc6f9cc

Sync with Prettier

0fd9380

Merge branch 'master' into fred/background-mode

ac3f00b

jayeb suggested changes Dec 7, 2018

View reviewed changes

frederickfogerty and others added 8 commits December 9, 2018 08:43

chore: update Background example to use Background

bd311b6

chore: remove extra param from imgix url

3a3b389

chore: refactor findNearestWidth to use a binary search for efficiency

c3b0d46

test: ensure dpr is rounded to 2dp

911f587

feat: ensure dpr is rounded to 2dp

e90efcb

Sync with Prettier

e88ce58

docs: update wording in README

0374cdf

frederickfogerty and others added 9 commits December 12, 2018 10:30

chore: remove browserstack reporter

f5d283a

chore: try downgrade karma

ef9372c

chore: try node 9

b93c6c0

test: use jsuri in tests instead of URL to support IE 11

3843d5a

Sync with Prettier

3772db3

chore: skip dpr override test on IE

6eb8e86

Merge branch 'fred/background-mode' of https://github.com/imgix/react…

044a61b

…-imgix into fred/background-mode * 'fred/background-mode' of https://github.com/imgix/react-imgix: Sync with Prettier

test: refactor test to see if it fixes IE

094f6d7

Sync with Prettier

04aaa97

jayeb approved these changes Dec 13, 2018

View reviewed changes

adamraider suggested changes Dec 20, 2018

View reviewed changes

adamraider and others added 4 commits December 21, 2018 11:03

chore: fix relative import

6663e47

Co-Authored-By: frederickfogerty <frederick.fogerty@gmail.com>

Sync with Prettier

e923c33

chore: fix toFixed

2edfc7a

chore: support SSR with background mode

17a74db

frederickfogerty and others added 7 commits December 21, 2018 11:11

test: skip dpr test in IE

9ec9660

test: fix tests on IE

18a4a43

Sync with Prettier

f1577f3

test: increase maxTries to try reduce flaky tests

8dd046d

Merge branch 'fred/background-mode' of https://github.com/imgix/react…

c75d7e2

…-imgix into fred/background-mode * 'fred/background-mode' of https://github.com/imgix/react-imgix: Sync with Prettier

test: fix url() trimming on mobile safari

e56107e

chore: remove unused file

7cc8a38

frederickfogerty merged commit 5c3ecf6 into master Dec 21, 2018

frederickfogerty deleted the fred/background-mode branch December 21, 2018 12:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add container to render image as background behind children [WIP] #236

feat: add container to render image as background behind children [WIP] #236

frederickfogerty commented Dec 4, 2018 •

edited

jayeb commented Dec 5, 2018

frederickfogerty commented Dec 6, 2018 •

edited

frederickfogerty commented Dec 6, 2018

frederickfogerty commented Dec 7, 2018

jayeb left a comment

jayeb Dec 7, 2018

frederickfogerty Dec 9, 2018

jayeb Dec 10, 2018

frederickfogerty Dec 10, 2018

jayeb Dec 7, 2018

frederickfogerty Dec 9, 2018

jayeb Dec 7, 2018

frederickfogerty Dec 9, 2018

frederickfogerty Dec 9, 2018

jayeb Dec 10, 2018

jayeb Dec 7, 2018

jayeb Dec 7, 2018

frederickfogerty Dec 9, 2018

adamraider Dec 20, 2018

adamraider Dec 20, 2018

frederickfogerty commented Dec 21, 2018 •

edited


		const findNearestWidth = actualWidth => findClosest(actualWidth, targetWidths);

		const toFixed = (dp, value) => +value.toFixed(2);

	const dpr = toFixed(2, imgixParams.dpr \|\| window.devicePixelRatio \|\| 1);
	const dpr = toFixed(2, imgixParams.dpr \|\| global.devicePixelRatio \|\| 1);

feat: add container to render image as background behind children [WIP] #236

feat: add container to render image as background behind children [WIP] #236

Conversation

frederickfogerty commented Dec 4, 2018 • edited

Description

TODO

Discussion Points

New Feature

Steps to Test

jayeb commented Dec 5, 2018

frederickfogerty commented Dec 6, 2018 • edited

frederickfogerty commented Dec 6, 2018

frederickfogerty commented Dec 7, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederickfogerty commented Dec 21, 2018 • edited

frederickfogerty commented Dec 4, 2018 •

edited

frederickfogerty commented Dec 6, 2018 •

edited

frederickfogerty commented Dec 21, 2018 •

edited