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: add ability to configure the srcset width tolerance #54

Merged
merged 9 commits into from
Nov 6, 2019

Conversation

sherwinski
Copy link
Contributor

@sherwinski sherwinski commented Nov 5, 2019

This PR adds logic which will allow users to modify the width tolerance used when building srcset width pairs. More specifically, the width tolerance dictates the maximum allowable percent difference between entries in a srcset attribute. By default, this rate is set to 8 percent. Users can use this setting to fine tune how many srcset pairs they generate when calling Imgix::Path#to_srcset.

A user can now specify their own tolerance rate by passing a percent value to the keyword parameter width_tolerance. See the following example:

client = Imgix::Client.new(host: 'testing.imgix.net', include_library_param: false)
client.path('image.jpg').to_srcset(width_tolerance: 0.20)

In this case, the width_tolerance is set to 20 percent, which will be reflected in the difference between subsequent widths in a srcset pair:

https://testing.imgix.net/image.jpg?w=100 100w,
https://testing.imgix.net/image.jpg?w=140 140w,
https://testing.imgix.net/image.jpg?w=196 196w,
							...
https://testing.imgix.net/image.jpg?w=8192 8192w

@sherwinski sherwinski requested a review from jayeb November 5, 2019 05:49
@@ -131,7 +131,10 @@ def has_query?
def build_srcset_pairs(params)
srcset = ''

for width in @target_widths do
width_tolerance = params.delete('width_tolerance'.to_sym)
widths = width_tolerance == DEFAULT_WIDTH_TOLERANCE ? @target_widths : TARGET_WIDTHS.call(width_tolerance)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jayeb The sort of gotcha to all this is that we already calculate the default TARGET_WIDTHS when we instantiate a new Path object. But if the user opts for a different width tolerance, we end up recalculating a new set of widths on the fly. This is obviously going to cause us to perform repeated work whenever a user wants to specify their own tolerance, but in the grand scheme of things I still don't want to be calculating the widths every time we call to_srcset
I'm toying around with the idea of storing the custom widths locally to avoid recalculating on subsequent calls to to_srcset(width_tolerance:x), but might just hold off until all other srcset features are built out.

Copy link
Contributor

@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.

The code all looks good. I'm not sure I agree with the decision to define this value as an integer, though. I would suggest we request the value as a unit scalar instead, so it better represents a percentage. This way we'd cut down on potential confusion where a user might think that the integer value they've provided represents an absolute value by which to scale widths, rather than a multiplier.

README.md Outdated Show resolved Hide resolved
@sherwinski sherwinski requested a review from jayeb November 5, 2019 23:23
@sherwinski sherwinski force-pushed the srcset-width-tolerance branch from 405c776 to d83e78d Compare November 6, 2019 08:08
@sherwinski sherwinski force-pushed the srcset-width-tolerance branch from d83e78d to 64395e4 Compare November 6, 2019 08:13
@sherwinski sherwinski changed the title feat: allow srcset width tolerance to be modified feat: add ability to configure the srcset width tolerance Nov 6, 2019
@sherwinski sherwinski merged commit eb5d4c8 into master Nov 6, 2019
@sherwinski sherwinski deleted the srcset-width-tolerance branch November 6, 2019 21:24
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.

2 participants