feat: dpr srcset options #307

sylcastaing · 2021-12-11T17:15:17Z

Description

This PR add 2 new options to control DPR srcset generation :

targetDPRRatios to generate DPR we need Allow configure srcSet DPR generation #304
targetDPRRatiosQualities to control image quality by DPR Allow different Quality presets for buildSrcSet #284

No breaking changes. buildSrcSet return the same as before if we don't provide this new options.

Checklist: New Feature

Each commit follows the Conventional Commit format: e.g. chore(readme): fix typo. See the end of this file for more information.
Any breaking changes are specified on the commit on which they are introduced with BREAKING CHANGE in the body of the commit.
If this is a big feature with breaking changes, consider opening an issue 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!

Steps to Test

`targetDPRRatios`

const client = new ImgixClient({
  domain: 'testing.imgix.net',
  secureURLToken: 'my-token',
  includeLibraryParam: false,
});

const srcset = client.buildSrcSet(
  'image.jpg',
  {
    h: 800,
    ar: '3:2',
    fit: 'crop',
  },
  {
    targetDPRRatios: [1, 2],
  },
);

Result :

https://testing.imgix.net/image.jpg?h=800&ar=3%3A2&fit=crop&dpr=1&s=3d754a157458402fd3e26977107ade74 1x,
https://testing.imgix.net/image.jpg?h=800&ar=3%3A2&fit=crop&dpr=2&s=a984ad1a81d24d9dd7d18195d5262c82 2x

`targetDPRRatiosQualities`

const client = new ImgixClient({
  domain: 'testing.imgix.net',
  includeLibraryParam: false,
});

const srcset = client.buildSrcSet(
  'image.jpg',
  { w: 100 },
  { targetDPRRatiosQualities: { 1: 45, 2: 30, 3: 20, 4: 15, 5: 10 } },
);

Result :

https://testing.imgix.net/image.jpg?w=100&dpr=1&q=45 1x,
https://testing.imgix.net/image.jpg?w=100&dpr=2&q=30 2x,
https://testing.imgix.net/image.jpg?w=100&dpr=3&q=20 3x,
https://testing.imgix.net/image.jpg?w=100&dpr=4&q=15 4x,
https://testing.imgix.net/image.jpg?w=100&dpr=5&q=10 5x

commit-lint · 2021-12-11T17:15:21Z

Features

dpr srcset options (961025b)

Bug Fixes

review: dpr srcset options (8d86dd9)

Chore

lint (b1fb340)

Contributors

sylcastaing, frederickfogerty

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

@Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
@Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
@Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
@Commit-Lint merge disable will desactivate merge dependabot PR
@Commit-Lint review will approve dependabot PR
@Commit-Lint stop review will stop approve dependabot PR

frederickfogerty

This looks really good to me at a first pass! The tests look great and thanks for updating the types and the documentation.

I'm debating a bit the naming of the parameters - maybe including target isn't necessary (even though we use it intentionally). We could potentially just use DPRRatios or manualDPRRatios to make things clearer. I will discuss with the team and come back to you on this.

Other than the comment I left below, this looks really great!

src/validators.js

luqven

Good work, this is looking great 👍🏼. I left a couple of minor comments.

luqven · 2021-12-13T14:22:39Z

README.md

+);
+```
+
+will generate the following custom `q` to `dpr` mapping:


Nit, let's capitalize this to keep it consistent.

Suggested change

will generate the following custom `q` to `dpr` mapping:

Will generate the following custom `q` to `dpr` mapping:

README.md

luqven · 2021-12-13T14:25:43Z

dist/index.cjs.js

@@ -45,6 +45,22 @@ function _objectSpread2(target) {
  return target;
 }

+function _typeof(obj) {


src/validators.js

sherwinski

Thanks for taking the time to do this @sylcastaing! A small nitpick from my end: we generally avoid checking in the dist/ directory in PRs as that is all handled during the release step.

frederickfogerty · 2021-12-14T18:48:35Z

@sylcastaing so I discussed with my team and we'd like to call the parameters devicePixelRatios instead of targetDPRs, and variableQualities rather than targetDPRRatiosQualities. This is the last change, and then we should be good to go!

sylcastaing · 2021-12-14T19:14:56Z

@sylcastaing so I discussed with my team and we'd like to call the parameters devicePixelRatios instead of targetDPRs, and variableQualities rather than targetDPRRatiosQualities. This is the last change, and then we should be good to go!

Ok thanks !

I have time tomorrow morning (Paris TZ) to make the last changes

frederickfogerty · 2021-12-14T20:19:03Z

Merci @sylcastaing, ca sonne bien ! À demain 👋

sylcastaing · 2021-12-15T14:12:00Z

I have push a new commit (not sure if the commit message is correct) with :

Rename options to devicePixelRatios and variableQualities
Accept float DPR
Remove dist file from PR

luqven

lgtm 👍🏼

Will let @frederickfogerty approve once he's had a chance to review.

src/index.js

src/validators.js

test/test-buildSrcSet.js

frederickfogerty

LGTM! Thanks so much for the contribution here @sylcastaing

# [3.3.0](v3.2.2...v3.3.0) (2021-12-16) ### Features * dpr srcset options ([#307](#307)) ([380abf0](380abf0)) [skip ci]

# [3.4.0](v3.2.2...v3.4.0) (2021-12-16) ### Features * dpr srcset options ([#307](#307)) ([380abf0](380abf0))

sylcastaing requested a review from a team as a code owner December 11, 2021 17:15

sylcastaing changed the title ~~feat: DPR srcset options~~ feat: dpr srcset options Dec 11, 2021

feat: dpr srcset options

961025b

sylcastaing force-pushed the feat/dpr-options branch from cd96a39 to 961025b Compare December 11, 2021 17:17

frederickfogerty suggested changes Dec 13, 2021

View reviewed changes

src/validators.js Outdated Show resolved Hide resolved

luqven suggested changes Dec 13, 2021

View reviewed changes

sherwinski suggested changes Dec 13, 2021

View reviewed changes

fix(review): dpr srcset options

8d86dd9

luqven reviewed Dec 15, 2021

View reviewed changes

src/index.js Outdated Show resolved Hide resolved

src/index.js Outdated Show resolved Hide resolved

src/validators.js Show resolved Hide resolved

test/test-buildSrcSet.js Show resolved Hide resolved

test/test-buildSrcSet.js Show resolved Hide resolved

chore: lint

b1fb340

frederickfogerty approved these changes Dec 16, 2021

View reviewed changes

frederickfogerty merged commit 380abf0 into imgix:main Dec 16, 2021

imgix-git-robot pushed a commit that referenced this pull request Dec 16, 2021

chore(release): 3.3.0

bdea3f5

# [3.3.0](v3.2.2...v3.3.0) (2021-12-16) ### Features * dpr srcset options ([#307](#307)) ([380abf0](380abf0)) [skip ci]

frederickfogerty pushed a commit that referenced this pull request Dec 16, 2021

chore(release): 3.4.0

61a6192

# [3.4.0](v3.2.2...v3.4.0) (2021-12-16) ### Features * dpr srcset options ([#307](#307)) ([380abf0](380abf0))

frederickfogerty pushed a commit that referenced this pull request Dec 16, 2021

chore(release): 3.4.0

1897efa

# [3.4.0](v3.2.2...v3.4.0) (2021-12-16) ### Features * dpr srcset options ([#307](#307)) ([380abf0](380abf0))

frederickfogerty mentioned this pull request Dec 28, 2021

Allow different Quality presets for buildSrcSet #284

Closed

sherwinski mentioned this pull request Jan 11, 2022

Allow configure srcSet DPR generation #304

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: dpr srcset options #307

feat: dpr srcset options #307

sylcastaing commented Dec 11, 2021 •

edited

Loading

commit-lint bot commented Dec 11, 2021 •

edited

Loading

frederickfogerty left a comment

luqven left a comment •

edited

Loading

luqven Dec 13, 2021

luqven Dec 13, 2021

sherwinski left a comment

frederickfogerty commented Dec 14, 2021

sylcastaing commented Dec 14, 2021

frederickfogerty commented Dec 14, 2021

sylcastaing commented Dec 15, 2021

luqven left a comment

frederickfogerty left a comment

	will generate the following custom `q` to `dpr` mapping:
	Will generate the following custom `q` to `dpr` mapping:

feat: dpr srcset options #307

feat: dpr srcset options #307

Conversation

sylcastaing commented Dec 11, 2021 • edited Loading

Description

Checklist: New Feature

Steps to Test

targetDPRRatios

targetDPRRatiosQualities

commit-lint bot commented Dec 11, 2021 • edited Loading

Features

Bug Fixes

Chore

Contributors

frederickfogerty left a comment

Choose a reason for hiding this comment

luqven left a comment • edited Loading

Choose a reason for hiding this comment

luqven Dec 13, 2021

Choose a reason for hiding this comment

luqven Dec 13, 2021

Choose a reason for hiding this comment

sherwinski left a comment

Choose a reason for hiding this comment

frederickfogerty commented Dec 14, 2021

sylcastaing commented Dec 14, 2021

frederickfogerty commented Dec 14, 2021

sylcastaing commented Dec 15, 2021

luqven left a comment

Choose a reason for hiding this comment

frederickfogerty left a comment

Choose a reason for hiding this comment

sylcastaing commented Dec 11, 2021 •

edited

Loading

`targetDPRRatios`

`targetDPRRatiosQualities`

commit-lint bot commented Dec 11, 2021 •

edited

Loading

luqven left a comment •

edited

Loading