twolfson / gulp.spritesmith

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

Wrong sprite dimensions #124

Closed jasonevines closed 7 years ago

jasonevines commented 7 years ago

When using the retina-sprite mixin, I get the wrong width and height for my sprites.

Let's say a sprite is supposed to appear 46px wide and high on the page. The retina-sprite mixin will use 92px for the CSS height and width instead, from the dimensions of the retina .png.

Here's the task I'm using to generate my sprites:

// Manage sprites gulp.task('sprites', function () { var spriteData = gulp.src([ imagePaths.source + "sprites/*.png", imagePaths.source + "sprites-2x/*.png" ]) .pipe(spritesmith({ retinaSrcFilter: imagePaths.source + "sprites-2x/*.png", imgName: 'spritesheet.png', retinaImgName: 'spritesheet-2x.png', cssName: '_sprites.scss' })) .on("error", handleError); var imgStream = spriteData.img .pipe(buffer()) .pipe(imagemin(imageMinConfig.pngQuant)) .pipe(gulp.dest(imagePaths.dist + 'sprites/')); var cssStream = spriteData.css .pipe(gulp.dest('./sass_helpers')); return merge(imgStream, cssStream); });

twolfson commented 7 years ago

The task looks correct. Can you paste how you're using the mixins?

jasonevines commented 7 years ago

Like this:

@include retina-sprite($icon-publication-dpdn-group);

twolfson commented 7 years ago

That looks correct as well. Can you provide the generated CSS and a screenshot of what you're expecting to see vs what you actually see?

jasonevines commented 7 years ago

Attached is the generated CSS (normally uses .scss, but using .txt for GitHub attachment).

_sprites.txt

Also attached are screenshots showing what the sprite icons should look, as generated by Compass (from which I'm migrating), and then as generated by Spritesmith.

normal-nav-screenshot

too-big-screenshot

twolfson commented 7 years ago

Oooooh, I see what's happening. I'm not sure how I missed this the first time. The normal and retina sprites have the same filename. As a result, we are overwriting the existing variable causing the retina mixin to look at the retina sprite as the normal sprite:

// Normal sprite
$books-name: 'books';
$books-x: 248px;
// ...
$books: (248px, 36px, -248px, -36px, 46px, 46px, 502px, 232px, 'spritesheet.png', 'books', );

// Retina sprite
$books-name: 'books';
$books-x: 496px;
// ...
$books: (496px, 72px, -496px, -72px, 92px, 92px, 1004px, 464px, 'spritesheet-2x.png', 'books', );

// Retina group
$books-group-name: 'books';
$books-group: ('books', $books, $books, );

We will definitely have to throw an error in our code but missed this before. As far as a workaround, there are a few options:

I'm going to leave this issue open until we get an error being thrown for using a normal and retina sprite with the same name

jasonevines commented 7 years ago

I get this error when trying the code you mentioned:

/.npm/node_modules/spritesheet-templates/node_modules/underscore/underscore.js:771 obj[prop] = source[prop]; ^

TypeError: Cannot set property 'bare_name' of undefined at /.npm/node_modules/spritesheet-templates/nodemodules/underscore/underscore.js:771:21 at Array.forEach (native) at .each._.forEach (/.npm/node_modules/spritesheet-templates/nodemodules/underscore/underscore.js:78:11) at Function..extend (/.npm/node_modules/spritesheet-templates/node_modules/underscore/underscore.js:768:5) at Function.templater.ensureHandlebarsVariables (/.npm/node_modules/spritesheet-templates/lib/spritesheet-templates.js:227:5) at addHandlebarsVariables (/.npm/node_modules/spritesheet-templates/lib/spritesheet-templates.js:282:17) at Array.forEach (native) at templateFn (/.npm/node_modules/spritesheet-templates/lib/spritesheet-templates.js:281:18) at templater (/.npm/node_modules/spritesheet-templates/lib/spritesheet-templates.js:118:16) at handleImages (/.npm/node_modules/gulp.spritesmith/lib/gulp-spritesmith.js:341:20) at /.npm/node_modules/gulp.spritesmith/node_modules/async/dist/async.js:3694:9 at /.npm/node_modules/gulp.spritesmith/node_modules/async/dist/async.js:359:16 at iteratorCallback (/.npm/node_modules/gulp.spritesmith/node_modules/async/dist/async.js:935:13) at /.npm/node_modules/gulp.spritesmith/node_modules/async/dist/async.js:843:16 at /.npm/node_modules/gulp.spritesmith/node_modules/async/dist/async.js:3691:13 at apply (/.npm/node_modules/gulp.spritesmith/node_modules/async/dist/async.js:21:25)

twolfson commented 7 years ago

Oh, whoops. I thought it was a return when it's a mutate. Try this one:

cssVarMap: function (sprite) { if (sprite.source_image.indexOf('2x') !== -1) { sprite.name += '-2x'; } }`
jasonevines commented 7 years ago

That's done the trick. Thanks for your help!

twolfson commented 7 years ago

We have created a test and patch in 6.5.0 to resolve this issue