Retina-Sprites-for-Compass icon indicating copy to clipboard operation
Retina-Sprites-for-Compass copied to clipboard

Allow multiple $sprites declarations

Open kriskhaira opened this issue 11 years ago • 8 comments

Hi Adam. The current helper only allows one $sprites declaration.

I usually have multiple sections in my apps where I'd like separate sprites files. For example, I'd like to be able to do this:

$ecommerce-sprites: sprite-map("ecommerce/sprites/*.png");
$ecommerce-sprites2x: sprite-map("ecommerce-retina/sprites/*.png");
$movie-sprites: sprite-map("movie/sprites/*.png");
$movie-sprites2x: sprite-map("movie-retina/sprites/*.png");

.paypal-btn {
  @include retina-sprite(paypal-btn, $ecommerce-sprites, $ecommerce-sprites2x);
}
.shopping-cart-btn {
  @include retina-sprite(shopping-cart-btn, $ecommerce-sprites, $ecommerce-sprites2x);
}

.play-icon {
  @include retina-sprite(play-icon, $movie-sprites, $movie-sprites2x);
}
.view-trailer-btn {
  @include retina-sprite(view-trailer-btn, $movie-sprites, $movie-sprites2x);
}

I was wondering if you'd like to merge this.

It would be cool if it would just use "$sprites" if it wasn't set; as well as appending "2x" to the name of the $sprites value if the $sprites2x parameter isn't defined; but that's beyond me at the moment.

kriskhaira avatar Jul 24 '13 14:07 kriskhaira

I should include an amended README.md in the pull request as well, but let me know your thoughts first. Thanks, Adam.

kriskhaira avatar Jul 24 '13 14:07 kriskhaira

Awesome, thanks for the PR @kriskhaira ! I'll dig into this and try it out Fri or this weekend! :beer:

Have you tried putting the sprite declarations in separate sass partials/files? In Tagit's website i'm using multiple sprites files with a single declaration in each, albeit much less explicit. example below.

I've also been thinking about changing the api to have one sprites folder and just have it look for a _2x or @2x, filename, which would require a breaking change.

_header.scss


/* ============================================================== *
 *                            Header
 * ============================================================== */


$sprites:   sprite-map("sprites/hdr/*.png");                // import 1x sprites
$sprites2x: sprite-map("sprites-retina/hdr/*.png");     // import retina sprites

...

.hdr-logo {
    float:left;
    margin-top: 16px;
    @include sprite(hdr-logo);
}

_buttons.scss


* ============================================================== *
 *  Buttons Module - .btn namespace
 * ============================================================== */

.btn {                             // Vector Buttons (default)
  ...
}

/* ====================  Raster Buttons  ==================== */

$sprites: sprite-map("sprites/btn/*.png");                 // import 1x sprites
$sprites2x: sprite-map("sprites-retina/btn/*.png");     // import retina sprites


.btn-playNow {
  @include sprite(playNow, $hover: true);
}

.btn-getTheApp {
  @include sprite(getTheApp);
}

.btn-signIn {
  @include sprite(signIn);
}

.btn-go {
  @include sprite(go, $hover: true);
}

.btn-registerNow {
  @include sprite(registerNow, $hover: true);
}

.btn-twitter {
  @include sprite(btn-twitter);
}
.btn-twitter-reg {
  @include sprite(btn-twitter-reg);
}

AdamBrodzinski avatar Jul 25 '13 02:07 AdamBrodzinski

Hey Adam, i've had the same problem and made multiple changes to your mixin over the time. I just uploaded these as a fork here: https://github.com/novascreen/Retina-mixins-for-Compass I didn't make a pull request because these changes would break things how they currently work. I created a retina-sprite-add mixin to be able to use multiple sprites and use sprite-url() only once per sprite instead of once per include (on a big project we just had too many calls to your mixin and when compiling it checked for changes in the sprites on every call) I also included support for IE8 with RespondJS and set a default spacing for sprites. Maybe you want to have a look and check out which of the changes you might want to include in your mixin.

novascreen avatar Jul 28 '13 22:07 novascreen

btw i like the idea of having file.png and [email protected] in one folder, but how would the sprite-map() calls for this look like?

novascreen avatar Jul 28 '13 22:07 novascreen

@novascreen Oh wow, so much awesomeness!! We could definitely pull it into a feature something branch, remove the respondjs dependency (if it's a hard dep) and then update to 2.0 once everything once it's smoothed out. I also love how clean it is =)

btw i like the idea of having file.png and [email protected] in one folder, but how would the sprite-map() calls for this look like?

To be honest i'm not sure offhand, i'd have to look over how the whole compass sprites works behind the scenes again... it's been almost a year :smile: . I was thinking of creating a mixin that would set both sprites as a side effect (like your retina-sprite-add), and perhaps take a param of just the path, e.g., retina-map('sprites/icons') and then append /*.png and /*@2x.png for each, assuming it would glob correctly.

AdamBrodzinski avatar Jul 29 '13 04:07 AdamBrodzinski

Cool, thanks for looking at this so quickly, i'm glad you like it.

RespondJS is not a hard dependency, i just had to support IE8 in many RWD projects and we used RespondJS to make that happen, so i wanted to have the option in there. But i have the option variable $retina-support-respondjs set to 0 by default, i guess there should be 2 separate demos to make that more clear.

I can refine this a little further, put it in a branch and then make a pull request, ok?

About [email protected]: My first thought was just this: "won't /*.png load /*@2x.png as well?", maybe we can find a way around that, but not sure if it's possible. We don't have regex ;)

But we could also just say it takes one path like sprites/icons and appends -retina for the retina icons. That would require the user to have a certain structure, not sure if that's desired. It's always hard to make it both simple and flexible.

novascreen avatar Jul 29 '13 04:07 novascreen

@novascreen

I can refine this a little further, put it in a branch and then make a pull request, ok?

Awesome! :beers:

RespondJS is not a hard dependency, i just had to support IE8 in many RWD projects and we used RespondJS to make that happen, so i wanted to have the option in there.

Oh yea I see now, at first I thought it wouldn't output if it wasn't detected... cool let's keep it in!

My first thought was just this: "won't /.png load /@2x.png as well?"

Bummer, yea you're right. Maybe we can ping Chris and Nate to see if there's a quick fix for this deep in sass/compass. If all else fails we could do as you suggested by but maybe have a retina-path param to override it if needed.

AdamBrodzinski avatar Jul 29 '13 05:07 AdamBrodzinski

Guys, sorry for the late reply.

@novascreen great work there. Would've used/forked your fork if I had seen it earlier.

I definitely support using "@2x" and allowing the files to be in the same folder instead of having Retina images in a separate folder.

One plus for having them in the same folder and using @2x is it's consistent with the way Slicy exports images. Slicy (http://macrabbit.com/slicy/) is an app which is used quite a lot to export images from PSD files based on the way you name the layers and groups. Right now I have to move all @2x files into a separate -retina folder and then run the following alias to remove the "@2x" from the filenames.

remove@2x='for f in $(find ./ -name "*@2x*"); do mv "$f" "${f//@2x/}"; done'

kriskhaira avatar Jul 31 '13 11:07 kriskhaira