-
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
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
8128785
add targetWidths()
14ab5fb
feat: add GenerateSrcSet() method to UrlBuilder
34d6c2b
test: add srcset tests
5db6f3f
docs: add srcset documentation
5e3d977
add dpr parameter to DPR srcset; handle conflict in adding ixlib para…
7f46399
test: fix broken tests
8d4a358
docs: fix typos
362f538
refactor: pull srcset width calculations into static class variable
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 toBuildUrl
. Curious if you think this strategy is preferable over say, changing L57 to useparameters["ixlib"] = ...
, which will avoid the same issue but requires touching on the originalBuildUrl
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
indiscriminatelyAdd
s anixlib
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 theparameters
(the dictionary/hash-map that is passed) already contain theixlib
key-value pair. If so, continue; otherwise, if the flag is set, but noixlib
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:
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:
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). CallingBuild
here should be the final step. No mutation needs to occur to any of the configuration values.