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

Added alt-function to support RetinaJS libraries. #21

Closed
wants to merge 6 commits into from
Closed

Added alt-function to support RetinaJS libraries. #21

wants to merge 6 commits into from

Conversation

mblarsen
Copy link

@mblarsen mblarsen commented Mar 4, 2014

This pull requests implements an alt(ernative) option. This options will allow to map the hash prefix from an original file - say logo.png => ab47.logo.png - onto alternative versions of that file - say logo@2x.png => ab47.logo@2x.png.

This is useful when you are working with retina images, that require the files to named identically except for some part of the filename, for example @2x.

Request included passing unit tests.

@steveluscher
Copy link

Continuing discussion from #15

Neat!

One bug and one API suggestion.

Bug: We must calculate the hash of the group of files. If I change one image or the other, the hash should change.

API suggestion: In my opinion, calculating a groupwise hash on files with the “@\dx” or “_\dx” suffix should be the default. Files will not likely get named as such by coincidence. Hence, I think the only option we need to add is a pattern, that when matched and eliminated, maps to some “root” filename.

Given this:

rev: {
  options: {
    // Default. RegExp or array of RegExps.
    // Matches @2x, _3x, @4x, _5x, etc…
    alternatesPattern: /[@_]\dx/
  },
  files: {
    src: ['**/*.{js,css,png,jpg}']
  }
}

The following filenames:

 image.jpg
 image@2x.jpg
 image@4x.jpg

…will map to this “root” filename:

image.jpg

All files in the group should then get hashed together to produce:

 {hash}.image.jpg
 {hash}.image@2x.jpg
 {hash}.image@4x.jpg

@mblarsen
Copy link
Author

mblarsen commented Mar 9, 2014

Good points. I'll add these changes.

@mblarsen
Copy link
Author

mblarsen commented Mar 9, 2014

I'll add an option to turn of handling of alternates.

@mblarsen
Copy link
Author

mblarsen commented Mar 9, 2014

I purposely used globs in stead of regexp, because I thought it would be easier for most. But the solution would be easier just using regexp.

I'm kind of for simplifying the solution.

Cons:

  • Slightly more complicated with regexp than globs
  • No separate pattern for matching alternates (will match all matching regexp)
  • Only one pattern of alternates, but most users will have only one, anyway and it could be expressed with pipe-sign.

Pros:

  • simpler config
  • less dependencies

The implementation is now using regexp for matching in stead of globs.
This makes the config simpler. Files are grouped and each group of files
gets a hash based on the content of all files in the group.
Removed dependency on glob-to-regexp and underscore.strings.
@mblarsen
Copy link
Author

mblarsen commented Mar 9, 2014

Check the new implementation. Works as to your specs.

@steveluscher
Copy link

Cool.

If it were me, I'd still have /[@_]\d+x/ be the default alternatesPattern, and I would not offer a flag for turning alternates on or off – simply set alternatesPattern to null to disable grouping.

alternatesPattern: /[@_]\d+x/            // Default
alternatesPattern: [/-hover/, /-active/] // An array of patterns
alternatesPattern: null                  // Turn alternate matching off    

Again, just so everyone's clear, the role of the pattern is to produce a match which, when removed from the filename, produces some ‘base’ filename that a group of alternates share in common. Those member files then get hashed as a group and the same hash gets applied to all of the files in the group.

So…

image.jpg
image-hover.jpg
image-active.jpg

…would all produce the base filename image.jpg after the matches produced by [/-hover/, /-active/] are removed.

Added tests for _default_ case
@mblarsen
Copy link
Author

I've removed alternates: true|false and replaced it with alternatesPattern: null

The reset worked already. Added test to show default case. There are not tests for array.

Note that [/-hover/, /-active/] can be achieved by /-(hover|active)/ as well, but some times it can make it more readable to split it up.

Probably should have been a major, 1.0.0, since it adds both new
features and breaks backward functionality.
@steveluscher
Copy link

Neat. Now we just need to get @CBas' opinion.

@mblarsen
Copy link
Author

Come to think of it. In the current implementation the array option is seen as patterns describing separate groups alternates - different schemes you could say:

alternatesPattern: [ /-hover/, /-active/ ]

Where as:

 alternatesPattern: [ /-(hover|active)/ ] // .. or
 alternatesPattern: /-(hover|active)/

Will would describe only one pattern of alternates.

A more practical use of array would be:

alternatesPattern: [ /-(hover|active)/, /[@_]\dx/ ]

The current implementation fails if two patterns matches the same file. An example

logo.png               // matched by both
logo-hover.png
logo-action.png
logo_2x.png
logo_4x.png

In real life I guess your pattern would be:

alternatesPattern: /-(hover|active)[@_]d\x/

@mblarsen
Copy link
Author

mblarsen commented Apr 4, 2014

Just to let you know. We are using this problem free in production now.@CBas @steveluscher

@steveluscher
Copy link

👍

@thiagofesta
Copy link

👍 I'd love to see it merged in and released as a new version... What we are waiting for? @CBas @steveluscher @mblarsen

@steveluscher
Copy link

I'm down, but I don't have write access to this repo. If this PR looks good to @CBas, it looks good to me!

@thiagofesta
Copy link

@CBas is not merging it, so for who wants to use it, replace grunt-rev with this one:

"grunt-rev": "https://github.com/TinyCarrier/grunt-rev/tarball/c8714c9a21ffd107c72a2a2db64cda9cfcd59f37"

This pull request was closed.
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.

3 participants