feat: add aspect ratio support by calculating client-side #201

frederickfogerty · 2018-10-10T04:17:10Z

Description

This PR allows the developer to specify a desired aspect ratio (AR) when creating an image.

This PR is based off the awesome work from @maxkolyanov!

This fixes #161

API

The developer specifies the AR along with normal imgix params, such as fit.

e.g.

<Imgix
  src={src}
  sizes="(max-width: 30em) 100vw, (max-width: 50em) 50vw"
  imgixParams={{ ar: "2:1" }}
/>

The AR has to be specified in the form number:number. When an invalid AR is detected, no height values will be generated, and a warning is thrown.

TODO

[N/A] Audit tests
Round h up to comply with API spec
Add Max as contributor

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: (29 commits) chore(deps): update dependency puppeteer to v1.9.0 (#200) chore(deps): update dependency sinon to v6.3.5 (#199) chore(deps): update dependency jest-extended to v0.11.0 (#198) chore(deps): update dependency sinon to v6 (#197) chore(deps): update dependency karma to v3 (#196) chore(deps): update dependency babel-eslint to v10 (#193) chore(deps): update jest monorepo to v23.6.0 (#191) chore(deps): update dependency webpack-cli to v3.1.2 (#190) chore(deps): update dependency webpack to v4.20.2 (#189) chore(deps): update dependency standard-version to v4.4.0 (#188) chore(deps): update dependency puppeteer to v1.8.0 (#187) chore(deps): update dependency jest-extended to v0.10.0 (#178) chore(deps): update dependency webpack-dev-server to v3.1.9 (#186) chore(deps): update dependency karma-webpack to v3.0.5 (#184) chore(deps): update dependency cross-env to v5.2.0 (#175) chore(deps): update dependency prettier to v1.14.3 (#182) chore(renovate): update automerge config (#185) chore(renovate): enable auto-merging (#183) chore(deps): update dependency babel-preset-env to v1.7.0 (#174) chore(travis): only run travis branch builds on master (#179) ...

ghost

Left some questions & comments 🙂

ghost · 2018-10-10T09:35:10Z

src/react-imgix.js

+
+  const [width, height] = aspectRatio.split(":");
+
+  return width / height;


Don't want to be nitpicky, but this feels a bit obscure. 💭
Is there a reason for not using something like parseFloat explicitly?

Are you saying this should be return parseFloat(width) / parseFloat(height)? I'm not quite understanding this comment

I think he's saying that relying on the implicit type conversion here feels a bit cloudy, and I agree. I'd rather see the parseFloat() calls in there, as you've stated.

That is exactly what I trying to say!
Sorry, ironically I phrased it weirdly as well.

ghost · 2018-10-10T09:46:40Z

src/react-imgix.js

+    return false;
+  }
+  const isValidFormat = str => !/^\d(\.\d+)?:\d(\.\d+)?$/.test(str);
+  if (isValidFormat(aspectRatio)) {


Does isValidFormat meant to be renamed to isInvalidFormat?
That ! in front of regex is a bit hard to notice, it looks like it is a part of the regex.

Alternatively, I'm in favor of moving ! to condition, something like:
if (!isValidFormat(aspectRatio)) { ...

Good catch 🙌

ghost · 2018-10-10T10:09:01Z

src/react-imgix.js

+ * Parse an aspect ratio in the format w:h to a decimal. If false is returned, the aspect ratio is in the wrong format.
+ */
+function parseAspectRatio(aspectRatio) {
+  if (typeof aspectRatio == null) {


This is potentially redundant.
It would be sufficient to make sure that aspectRatio is a string.
So check below would be sufficient on its own.

ghost · 2018-10-10T10:18:50Z

src/react-imgix.js

          ...srcOptions,
          width: targetWidth
-        });
+        };
+        const aspectRatioDecimal = aspectRatio && parseAspectRatio(aspectRatio);


Following on comments on type checks in parseAspectRatio;
It seems to be sufficient to just pass aspectRatio into parseAspectRatio and throw warning if false was returned.

ghost · 2018-10-10T10:20:10Z

src/react-imgix.js

-        });
+        };
+        const aspectRatioDecimal = aspectRatio && parseAspectRatio(aspectRatio);
+        if (aspectRatio != null && aspectRatioDecimal === false) {


See above.
If all type checks just happen inside parseAspectRatio, aspectRatio != null can be removed.

The reason for having aspectRatio != null here is so that a warning is only thrown when the user has tried to pass something. e.g. it wouldn't throw a warning when no prop is passed, but it would when the user passes "". Do you think it's still valid? @maxkolyanov

Good call! Totally makes sense to check if the prop is there, before showing a warning.
One question I guess is wouldn't it be equal to undefined then instead of null, if the prop is absent/not passed?

Definitely! This is the reason for using != rather than !==, as this covers both undefined and null.

i.e.

undefined != null // false -- this is what we want undefined !== null // true -- this would break the conditional check

Does this make sense?

Totally! Great work on PR, LGTM 👍

Thanks @maxkolyanov! 🙌 thanks again for your work for the base for this PR.

ghost · 2018-10-10T10:23:09Z

src/react-imgix.js

@@ -38,6 +38,7 @@ const COMMON_PROP_TYPES = {

 const SHARED_IMGIX_AND_SOURCE_PROP_TYPES = {
  ...COMMON_PROP_TYPES,
+  aspectRatio: PropTypes.number,


It seems like aspectRatio is a string

This was fixed in a commit I didn't push 😬

jayeb · 2018-10-11T20:21:36Z

src/react-imgix.js

+  if (typeof aspectRatio !== "string") {
+    return false;
+  }
+  const isValidFormat = str => /^\d(\.\d+)?:\d(\.\d+)?$/.test(str);


This doesn't allow inputs with more than one digit left of the decimal point, right? Should be /^\d+(\.\d+)?:\d+(\.\d+)?$/, I think.

Good catch!

Can we also update the unit tests to make sure this is covered?

Yep, all done!

jayeb · 2018-10-11T20:23:30Z

src/react-imgix.js

+
+  const [width, height] = aspectRatio.split(":");
+
+  return width / height;


I think he's saying that relying on the implicit type conversion here feels a bit cloudy, and I agree. I'd rather see the parseFloat() calls in there, as you've stated.

jayeb · 2018-10-11T20:25:42Z

src/react-imgix.js

+          aspectRatioDecimal &&
+          aspectRatioDecimal > 0
+        ) {
+          urlParams.height = Math.round(targetWidth / aspectRatioDecimal);


After discussing this with some other engineers internally, this should be Math.ceil() to match the behavior in our backend when rounding dimensions.

frederickfogerty · 2018-10-11T21:49:30Z

@maxkolyanov @jayeb I think I've covered everything that was brought up. Can I please get another quick review?

minortext and others added 6 commits September 11, 2018 18:20

feat: introduce aspectRatio prop

70b75ca

test: add test for aspectRatio prop

5c56d67

test: add tests for AR implementation

218d48f

feat: implement AR

aac30b7

feat: add warning when invalid AR is passed

f6dd01f

frederickfogerty requested a review from jayeb October 10, 2018 04:17

frederickfogerty added 2 commits October 10, 2018 17:21

docs: add info about AR

2cde699

fix: remove incorrect aspectRatio prop

fa27b2c

frederickfogerty mentioned this pull request Oct 10, 2018

feat: aspectRatio prop #165

Closed

9 tasks

ghost suggested changes Oct 10, 2018

View reviewed changes

frederickfogerty added 4 commits October 12, 2018 08:05

chore: add Max as contributor

8061dcb

chore: refactor conditional to be clearer

25fbc09

chore: remove unnecessary check

1f78e3e

chore: remove unnecessary check

9161d0f

jayeb suggested changes Oct 11, 2018

View reviewed changes

frederickfogerty added 3 commits October 12, 2018 10:44

chore: fix validation to allow more than one number before full stop

71eaac1

feat: ceil height to match backend rendering

841fbf1

chore: use parseFloat explicitly when calculating AR

001be97

jayeb approved these changes Oct 11, 2018

View reviewed changes

frederickfogerty merged commit 7ce0411 into master Oct 11, 2018

frederickfogerty deleted the fred/aspect-ratio branch October 11, 2018 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add aspect ratio support by calculating client-side #201

feat: add aspect ratio support by calculating client-side #201

frederickfogerty commented Oct 10, 2018 •

edited

Loading

ghost left a comment

ghost Oct 10, 2018

frederickfogerty Oct 11, 2018

jayeb Oct 11, 2018

ghost Oct 11, 2018 •

edited by ghost

Loading

ghost Oct 10, 2018

frederickfogerty Oct 11, 2018

ghost Oct 10, 2018

ghost Oct 10, 2018

ghost Oct 10, 2018

frederickfogerty Oct 11, 2018 •

edited

Loading

ghost Oct 11, 2018 •

edited by ghost

Loading

frederickfogerty Oct 11, 2018 •

edited

Loading

ghost Oct 11, 2018

frederickfogerty Oct 11, 2018

ghost Oct 10, 2018

frederickfogerty Oct 11, 2018

jayeb Oct 11, 2018

frederickfogerty Oct 11, 2018

jayeb Oct 11, 2018

frederickfogerty Oct 11, 2018

jayeb Oct 11, 2018

jayeb Oct 11, 2018

frederickfogerty commented Oct 11, 2018


		const [width, height] = aspectRatio.split(":");

		return width / height;

feat: add aspect ratio support by calculating client-side #201

feat: add aspect ratio support by calculating client-side #201

Conversation

frederickfogerty commented Oct 10, 2018 • edited Loading

Description

API

TODO

New Feature

Steps to Test

ghost 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

ghost Oct 11, 2018 • edited by ghost 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 Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

ghost Oct 11, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

frederickfogerty Oct 11, 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

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 Oct 11, 2018

frederickfogerty commented Oct 10, 2018 •

edited

Loading

ghost Oct 11, 2018 •

edited by ghost

Loading

frederickfogerty Oct 11, 2018 •

edited

Loading

ghost Oct 11, 2018 •

edited by ghost

Loading

frederickfogerty Oct 11, 2018 •

edited

Loading