-
Notifications
You must be signed in to change notification settings - Fork 8
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 srcset generation #25
Conversation
foreach(int ratio in targetRatios) | ||
{ | ||
parameters["dpr"] = ratio.ToString(); | ||
parameters.Remove("ixlib"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zacman85 Looks like the way we're adding ixlib
(L57) is causing a collision error here unless we "remove" it between calls to BuildUrl
. Curious if you think this strategy is preferable over say, changing L57 to use parameters["ixlib"] = ...
, which will avoid the same issue but requires touching on the original BuildUrl
implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I do not like to have to remove things from a list like this, when something is being built up, unless it is unavoidable. I don't know enough about the codebase to suggest one way over the other as I don't know the consequences of a refactor here. I would leave it to your judgement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The immediate issue here is that BuildUrl
indiscriminately Add
s an ixlib
parameter if the flag has been set. The real issues are more nuanced...
Nonetheless, Remove
can be removed from the codebase if we just check if the parameters
(the dictionary/hash-map that is passed) already contain the ixlib
key-value pair. If so, continue; otherwise, if the flag is set, but no ixlib
param is present, Add
it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also go a bit further than this:
In general, I do not like to have to remove things from a list like this, when something is being built up, unless it is unavoidable.
We're at the build step. All configuration should have stopped by this point.
The builder pattern is a great way of separating "configuration" from "behavior." For instance, we should start thinking about what these libraries do as a three step process:
- initialize
- configure
- construct
Initialization describes the purpose of language-level constructors. Configuration describes the steps taken to configure the data within the initialized data structure (object, etc.). Construction describes the process of "putting it all together." Note that this last step boils down to concatenating or joining the configuration data.
The problem with functions like BuildUrl
is that they abuse abstraction barriers. Here, this function does a little configuration then a little construction (or delegation to some function that ultimately constructs the result). Calling Build
here should be the final step. No mutation needs to occur to any of the configuration values.
foreach(int ratio in targetRatios) | ||
{ | ||
parameters["dpr"] = ratio.ToString(); | ||
parameters.Remove("ixlib"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I do not like to have to remove things from a list like this, when something is being built up, unless it is unavoidable. I don't know enough about the codebase to suggest one way over the other as I don't know the consequences of a refactor here. I would leave it to your judgement.
This PR creates a new class method in
UrlBuilder
that will generate asrcset
string, which can be used in thesrcset
attribute on an<img>
HTML element.BuildSrcSet()
takes apath
andparams
argument similar toBuildUrl()
, and will return aString
in one of twosrcset
formats.Srcset Width-Pairs
The first format creates
srcset
width-pairs, a comma-delimited list of URLs and width descriptors corresponding to each one. This allows for responsive size switching based on the current width of the browser viewport.BuildSrcSet()
will append anyparams
passed to the method to each URL constructed within the list. The following is an example of how to generate a width-pairsrcset
:will return the following
String
(collapsed for brevity):Notice that a
w=
parameter is automatically inserted for each entry in thesrcset
list. This is required in order for the element to properly display the correctly-sized image corresponding to the viewport's width.Device Pixel Ratio (DPR) Srcset
The second format this method can return is a
srcset
list of same-size images in varying resolutions. In this case, images are scaled using thedpr=
parameter to adjust for the device pixel ratio of the browser. A DPRsrcset
will be automatically generated instead of a width-pair srcset if exact dimensions for the output image are specified, by providing either aw
(width) or ah
(height) andar
(aspect ratio) in theparametes
argument.The following is an example of how to generate a DPR
srcset
:which will return the following
String
:Signing URLs
BuildSrcSet()
also supports signing URLs, which can be very useful for generating server-side and then passing to the client. This will avoid having to expose your imgix source's secure token to the client.generates a
srcset
with unique signatures for each URL:For more information on
srcset
, building responsive images, and resolution switching: