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: use exponential increase for srcset widths #224

Merged
merged 7 commits into from
Nov 26, 2018

Conversation

frederickfogerty
Copy link
Contributor

@frederickfogerty frederickfogerty commented Nov 17, 2018

Description

This PR updates the srcset width-generation logic. The widths now grow exponentially, rather than by a fixed amount. This means that the dimensions of a rendered image are at most 8% from the "pixel-perfect" dimensions.

Fixes #227

Steps to test

Ensure that tests still pass.

Copy link

@jayeb jayeb left a comment

Choose a reason for hiding this comment

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

Huh, I thought this change landed months ago! I must have gotten it confused with changes to Ember.

Looks good to me. Will look again once the tests land.

@frederickfogerty
Copy link
Contributor Author

@jayeb seems like the tests have passed - is this good to go?

@jayeb
Copy link

jayeb commented Nov 20, 2018

Ah, I saw something about tests in the description and assumed there would be tests coming to cover the new behavior. Could we get some?

@frederickfogerty
Copy link
Contributor Author

Similar to the comment over on imgix.js (imgix/imgix.js#130), not sure what tests I can add which don't over-specify on the implementation.

…om/imgix/react-imgix into fred/use-exponential-srcset-widths

* 'fred/use-exponential-srcset-widths' of https://github.com/imgix/react-imgix:
  chore(deps): update dependency webpack to v4.26.0 (#226)
  chore(deps): update react monorepo to v16.6.3 (#223)
  chore(deps): update dependency prettier to v1.15.2 (#222)
@frederickfogerty
Copy link
Contributor Author

Hey @jayeb I've added some more tests as discussed offline - can you have a look?

Copy link

@jayeb jayeb left a comment

Choose a reason for hiding this comment

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

Great tests, thank you.

@frederickfogerty frederickfogerty merged commit bc5660c into master Nov 26, 2018
@frederickfogerty frederickfogerty deleted the fred/use-exponential-srcset-widths branch November 26, 2018 22:40
frederickfogerty added a commit that referenced this pull request Dec 4, 2018
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove "fallback src" in srcset generation
2 participants