twolfson / gulp.spritesmith

Convert a set of images into a spritesheet and CSS variables via gulp
The Unlicense
1.08k stars 81 forks source link

Potential issue when outputting .scss files #40

Closed jasonmerino closed 9 years ago

jasonmerino commented 9 years ago

I'm not sure if this is an issue in spritesmith or gulp.spritesmith, but I've been getting some funny output when trying to generate .scss files for the sprite variables. On the line where the variable is just the name of the image (in my example below $jedi) there is a trailing comma before the closing paren (line 24).

test

Now this isn't causing issues on my development machine (OSX) but on a CI build machine (CentOS) it's not allowing sass to compile it) Here is a demo I setup to test out that this is indeed the output.

Let me know if this isn't the right place to have this fixed and I'll take it up with someone else. Thanks!

twolfson commented 9 years ago

Someone recently reported the same issue in https://github.com/twolfson/spritesheet-templates/issues/39

What version of SASS are you running?

sass --version
jasonmerino commented 9 years ago

sass (3.4.13, 3.2.14)

jasonmerino commented 9 years ago

Oh! But the machine that is having trouble is sass (3.2.14)

twolfson commented 9 years ago

That is approximately 1 year old. Would you consider upgrading?

http://sass-lang.com/documentation/file.SASS_CHANGELOG.html#3214_24_january_2014

jasonmerino commented 9 years ago

My apologies. I forgot for a second that we are now using gulp-sass which in turn uses node-sass which leverages libsass which is only just nearing feature parity with sass v3.2... So unfortunately any upgrade of libsass is not really possible right now...

And switching away from libsass isn't really possible as it has taken our sass build time from ~21 seconds to <2 seconds.

twolfson commented 9 years ago

Our tests are running against libsass so that shouldn't be the problem =/

https://github.com/twolfson/spritesheet-templates/blob/9.4.0/test/scss_test.js#L42-L52

https://github.com/twolfson/spritesheet-templates/blob/9.4.0/.travis.yml#L28-L40

jasonmerino commented 9 years ago

Oh cool! Looks like the tests run against libsass 3.0.2. Do you think that running them against 3.1.0 would have any effect on this?

twolfson commented 9 years ago

I don't quite understand your question =/

jasonmerino commented 9 years ago

I was wondering if the test results changed if it were run against libsass 3.1.0 since it looks like it's running against 3.0.2. Maybe it wouldn't, I'm not super familiar with libsass I must admit.

twolfson commented 9 years ago

Yea, I have no idea either. I am not sure when I will be free to look at this but I will take a look by the end of next weekend.

twolfson commented 9 years ago

I figured a triage would get us there faster. I added a test to try to reproduce the error but it seems to be passing. I am using the latest version of node-sass (2.1.1) which is encompassed by the latest gulp-sass.

https://github.com/twolfson/spritesheet-templates/blob/fbb4b323cfe4c1190c7d836ba9753bfcac518e2d/test/scss_test.js#L56-L68

https://github.com/twolfson/spritesheet-templates/blob/fbb4b323cfe4c1190c7d836ba9753bfcac518e2d/package.json#L50

https://github.com/dlmanning/gulp-sass/blob/v1.3.3/package.json#L27

I have also tried out versions down to 1.0.3 without any issue:

https://github.com/twolfson/spritesheet-templates/blob/1da568f74703151778172f07da7b84f594108762/package.json#L50

What version of gulp-sass and node-sass are you using?

npm ls gulp-sass
npm ls node-sass
twolfson commented 9 years ago

Ah, I have caught it. gulp-sass was initially released on node-sass@0.6.

https://github.com/dlmanning/gulp-sass/commit/a983cb73201bcdb5f9d5e09899312ce3ce436994#diff-b9cfc7f2cdf78a7f4b91a753d10865a2R22

It looks like node-sass@0.9 works but node-sass@0.8 doesn't.

twolfson commented 9 years ago

Hmm, node-sass@0.9 was released in Jun 2014 but the last node-sass@0.8 was published in Apr 2014. Can you consider upgrading?

https://github.com/sass/node-sass/tree/v0.9.0

https://github.com/sass/node-sass/tree/v0.8.6

qikkeronline commented 9 years ago

@twolfson I'm having the same issue with grunt-sass, running on libsass 3.1.0; the error I'm getting is as following:

index out of bounds for `nth($list, $n)`
Line 68  Column 18
../webroot/wp-content/themes/qikkeronline/assets/sass/variables/sprites.scss

The lines in question are using the nth function; and as far as I can debug it is related to libsass not supporting maps in the nth function.

I'm using the mixin as following:

@include sprite($logo-name);

And my generated scss file:

/*
SCSS variables are information about icon's compiled state, stored under its original file name

.icon-home {
  width: $icon-home-width;
}

The large array-like variables contain all information about a single icon
$icon-home: x y offset_x offset_y width height total_width total_height image_path;

At the bottom of this section, we provide information about the spritesheet itself
$spritesheet: width height image $spritesheet-sprites;
*/
$logo-name: 'logo';
$logo-x: 0px;
$logo-y: 0px;
$logo-offset-x: 0px;
$logo-offset-y: 0px;
$logo-width: 459px;
$logo-height: 95px;
$logo-total-width: 459px;
$logo-total-height: 123px;
$logo-image: '../img/';
$logo: (0px, 0px, 0px, 0px, 459px, 95px, 459px, 123px, '../img/', 'logo', );
$twitter-icon-name: 'twitter-icon';
$twitter-icon-x: 0px;
$twitter-icon-y: 95px;
$twitter-icon-offset-x: 0px;
$twitter-icon-offset-y: -95px;
$twitter-icon-width: 34px;
$twitter-icon-height: 28px;
$twitter-icon-total-width: 459px;
$twitter-icon-total-height: 123px;
$twitter-icon-image: '../img/';
$twitter-icon: (0px, 95px, 0px, -95px, 34px, 28px, 459px, 123px, '../img/', 'twitter-icon', );
$spritesheet-width: 459px;
$spritesheet-height: 123px;
$spritesheet-image: '../img/';
$spritesheet-sprites: ($logo, $twitter-icon, );
$spritesheet: (459px, 123px, '../img/', $spritesheet-sprites, );

/*
The provided mixins are intended to be used with the array-like variables

.icon-home {
  @include sprite-width($icon-home);
}

.icon-email {
  @include sprite($icon-email);
}
*/
@mixin sprite-width($sprite) {
  width: nth($sprite, 5);
}

@mixin sprite-height($sprite) {
  height: nth($sprite, 6);
}

@mixin sprite-position($sprite) {
  $sprite-offset-x: nth($sprite, 3);
  $sprite-offset-y: nth($sprite, 4);
  background-position: $sprite-offset-x  $sprite-offset-y;
}

@mixin sprite-image($sprite) {
  $sprite-image: nth($sprite, 9);
  background-image: url(#{$sprite-image});
}

@mixin sprite($sprite) {
  @include sprite-image($sprite);
  @include sprite-position($sprite);
  @include sprite-width($sprite);
  @include sprite-height($sprite);
}

/*
The `sprites` mixin generates identical output to the CSS template
  but can be overridden inside of SCSS

@include sprites($spritesheet-sprites);
*/
@mixin sprites($sprites) {
  @each $sprite in $sprites {
    $sprite-name: nth($sprite, 10);
    .#{$sprite-name} {
      @include sprite($sprite);
    }
  }
}

Please let me know if I can be of more help; new to debugging this kind of stuff :)

twolfson commented 9 years ago

You are using the name variable for the sprite. Not the sprite list variable itself. If you update your include to use $logo instead, then everything should work again:

@include sprite($logo);
twolfson commented 9 years ago

Whoops, didn't mean to close this. Unrelated to the original issue x_x

qikkeronline commented 9 years ago

Thanks @twolfson - noob mistake on my part :+1:

twolfson commented 9 years ago

I think I am going to close this issue as a wontfix. It feels like since the supported libsass version has been out for over a year we can request that users upgrade to it. Thanks for all the feedback everyone =)

Vagelis-Prokopiou commented 9 years ago

Hello. I am having a similar problem and I did not manage to figure it out with has already been written. My gulp task is this: // Create the sprite. gulp.task('sprite', function () { var spriteData = gulp.src('images/*/').pipe(spritesmith({ imgName: 'sprite.jpg', cssName: '_sprite.scss', cssFormat: 'scss', imgPath: '../images/sprite.jpg'
})); // Choose css destination. var cssStream = spriteData.css .pipe(gulp.dest('./sass')); // Choose img destination.
return spriteData.img.pipe(gulp.dest('images')); });

My gulp-sass and node-sass are these: gulp-sass@2.1.0 └── node-sass@3.4.2 Yet, in the terminal I get this error: Message: sass/_sprite.scss Error: Invalid CSS after "*/": expected 1 selector or at-rule, was "$2-name: '2';" on line 13 of sass/_sprite.scss

*/ --^

The produced _sprite.scss is this: /* SCSS variables are information about icon's compiled state, stored under its original file name

.icon-home { width: $icon-home-width; }

The large array-like variables contain all information about a single icon $icon-home: x y offset_x offset_y width height total_width total_height image_path;

At the bottom of this section, we provide information about the spritesheet itself $spritesheet: width height image $spritesheet-sprites; */ $2-name: '2'; $2-x: 0px; $2-y: 6170px; $2-offset-x: 0px; $2-offset-y: -6170px; $2-width: 1833px; $2-height: 1152px; $2-total-width: 9355px; $2-total-height: 7322px; $2-image: '../images/sprite.jpg'; $2: (0px, 6170px, 0px, -6170px, 1833px, 1152px, 9355px, 7322px, '../images/sprite.jpg', '2', ); $3-name: '3'; $3-x: 8385px; $3-y: 0px; $3-offset-x: -8385px; $3-offset-y: 0px; $3-width: 970px; $3-height: 1562px; $3-total-width: 9355px; $3-total-height: 7322px; $3-image: '../images/sprite.jpg'; $3: (8385px, 0px, -8385px, 0px, 970px, 1562px, 9355px, 7322px, '../images/sprite.jpg', '3', ); $logo-name: 'logo'; $logo-x: 8385px; $logo-y: 1562px; $logo-offset-x: -8385px; $logo-offset-y: -1562px; $logo-width: 946px; $logo-height: 1152px; $logo-total-width: 9355px; $logo-total-height: 7322px; $logo-image: '../images/sprite.jpg'; $logo: (8385px, 1562px, -8385px, -1562px, 946px, 1152px, 9355px, 7322px, '../images/sprite.jpg', 'logo', ); $sprite-name: 'sprite'; $sprite-x: 0px; $sprite-y: 0px; $sprite-offset-x: 0px; $sprite-offset-y: 0px; $sprite-width: 8385px; $sprite-height: 6170px; $sprite-total-width: 9355px; $sprite-total-height: 7322px; $sprite-image: '../images/sprite.jpg'; $sprite: (0px, 0px, 0px, 0px, 8385px, 6170px, 9355px, 7322px, '../images/sprite.jpg', 'sprite', ); $spritesheet-width: 9355px; $spritesheet-height: 7322px; $spritesheet-image: '../images/sprite.jpg'; $spritesheet-sprites: ($2, $3, $logo, $sprite, ); $spritesheet: (9355px, 7322px, '../images/sprite.jpg', $spritesheet-sprites, );

/* The provided mixins are intended to be used with the array-like variables

.icon-home { @include sprite-width($icon-home); }

.icon-email { @include sprite($icon-email); } */ @mixin sprite-width($sprite) { width: nth($sprite, 5); }

@mixin sprite-height($sprite) { height: nth($sprite, 6); }

@mixin sprite-position($sprite) { $sprite-offset-x: nth($sprite, 3); $sprite-offset-y: nth($sprite, 4); background-position: $sprite-offset-x $sprite-offset-y; }

@mixin sprite-image($sprite) { $sprite-image: nth($sprite, 9); background-image: url(#{$sprite-image}); }

@mixin sprite($sprite) { @include sprite-image($sprite); @include sprite-position($sprite); @include sprite-width($sprite); @include sprite-height($sprite); }

/* The sprites mixin generates identical output to the CSS template but can be overridden inside of SCSS

@include sprites($spritesheet-sprites); */ @mixin sprites($sprites) { @each $sprite in $sprites { $sprite-name: nth($sprite, 10); .#{$sprite-name} { @include sprite($sprite); } } }

Can anybody suggest what is wrong? Thanx in advance…

twolfson commented 9 years ago

@Vaggos The issue in your setup is SASS doesn't support variables with leading numbers (e.g. $a is fine but $2 is not). To resolve your issue, there are a couple options:

Vagelis-Prokopiou commented 9 years ago

Thanx a lot @twolfson!!! Indeed, renaming the images solved the problem... But, where is this written? In the SASS documentation? I have never seen it anywhere.

twolfson commented 9 years ago

Yea, it doesn't seem to be in the documentation but a quick trial/error on Sassmeister (CodePen would have worked too) showed me one had a compile error (i.e. $2) and the other didn't (i.e. $a) =/

http://sassmeister.com/

Vagelis-Prokopiou commented 9 years ago

Thanx man for the info. You helped a lot. On Nov 17, 2015 7:35 PM, "Todd Wolfson" notifications@github.com wrote:

Yea, it doesn't seem to be in the documentation but a quick trial/error on Sassmeister (CodePen would have worked too) showed me one had a compile error (i.e. $2) and the other didn't (i.e. $a) =/

http://sassmeister.com/

— Reply to this email directly or view it on GitHub https://github.com/twolfson/gulp.spritesmith/issues/40#issuecomment-157446739 .