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

Allow multiple $sprites declarations #20

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kriskhaira
Copy link
Contributor

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
Copy link
Contributor Author

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

@AdamBrodzinski
Copy link
Owner

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

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);
}

@novascreen
Copy link
Collaborator

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
Copy link
Collaborator

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?

@AdamBrodzinski
Copy link
Owner

@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 😄 . 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.

@novascreen
Copy link
Collaborator

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.

@AdamBrodzinski
Copy link
Owner

@novascreen

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

Awesome! 🍻

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.

@kriskhaira
Copy link
Contributor Author

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'

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