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

Add dpr setting to processImages transform #158

Merged
merged 1 commit into from
Jan 4, 2015
Merged

Conversation

Munter
Copy link
Member

@Munter Munter commented Dec 30, 2014

Relates to assetgraph/assetgraph-sprite#13 (comment)

Add support for setting the device pixel ratio of the image asset that is the result of a processImage run.
The idea here is to be able to set the device pixel ratio as part of the image pipeline without having to set the device pixel ration in the filename itself, given that the file name cannot contain the @2x syntax if the image pipeline is what creates the image.

I've changed the behavior so dpr=/\d+(?:[,\.]\d+)?/ gets removed from the url during the transform, and the corresponding device pixel ratio is applied to the resulting new asset or the existing asset if no new one is created.

This means that more of the code in the transform is now run than before, since the whole used/unused query string logic was only in use if filters were applied. Now these parts are in play for every asset.

@papandreou what do you think of this?

Ping @silvenon

@coveralls
Copy link

Coverage Status

Coverage increased (+0.25%) when pulling 11025fd on processImages-dpr into d9acf3d on master.

@silvenon
Copy link
Contributor

silvenon commented Jan 1, 2015

device pixel ration

I also accidentally type "ration" instead of "ratio", always. I have no idea why that is happening 😃

given that the file name cannot contain the @2x syntax if the image pipeline is what creates the image.

Will something bad happen if the filename is using the @ syntax? It will just not be parsed as a dpr, right?

I've changed the behavior so dpr=/\d+(?:[,\.]\d+)?/ gets removed from the url during the transform, and the corresponding device pixel ratio is applied to the resulting new asset or the existing asset if no new one is created.

So it would be used like this?

.foo {
  background-image: url(images/bar.png?resize=32);
  background-size: 32px;
}

@media (-webkit-min-device-pixel-ratio: 2), (min-resolution: 192dpi) {
  .foo {
    background-image: url(images/bar.png?resize=64&dpr=2);
  }
}

Also, the order of the dpr parameter doesn't matter, right?

@Munter
Copy link
Member Author

Munter commented Jan 1, 2015

Will something bad happen if the filename is using the @ syntax? It will just not be parsed as a dpr, right?

It will, but in a different part of the life cycle. If a file has such a name the dpr will be set upon creation of the Image asset in the graph. if it has ?dpr=2 that ratio will be set when the processImages transform is run.

Your example is exactly how it would work.

Also, the order of the dpr parameter doesn't matter, right?

Correct. I tried thinking about how to take dpr into account as a modifier of following operations, but gave up on it, since most of this logic resides outside the scope of assetgraph and thus has no knowledge of dpr.

@silvenon
Copy link
Contributor

silvenon commented Jan 1, 2015

Ok, got it.

@Munter
Copy link
Member Author

Munter commented Jan 4, 2015

Allright, I haven't come up with any reason not to add this patch. Merging

Munter added a commit that referenced this pull request Jan 4, 2015
Add dpr setting to processImages transform
@Munter Munter merged commit 45edcdd into master Jan 4, 2015
@Munter
Copy link
Member Author

Munter commented Jan 4, 2015

Released in:

  • assetgraph-builder 2.10.0
  • grunt-reduce 1.3.0
  • generator-greenfield 1.5.0

@silvenon
Copy link
Contributor

silvenon commented Jan 9, 2015

👍 👍 👍

@Munter Munter deleted the processImages-dpr branch January 9, 2015 12:44
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.

None yet

3 participants