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

Default sort behaviour in any algorithm easily breaks retina versions #137

Closed
Sebobo opened this issue Jul 23, 2015 · 15 comments
Closed

Default sort behaviour in any algorithm easily breaks retina versions #137

Sebobo opened this issue Jul 23, 2015 · 15 comments

Comments

@Sebobo
Copy link

Sebobo commented Jul 23, 2015

I spent quite some time now to find the reason why some of the retina versions of the sprites
were wrong.
Reason is that by default the layout algorithm sorts the sprites and in my case
the sorting was different in standard and retina. But the scss mixins currently expect the sprites
to be at the same offset.

Can this be changed to sort:false by default or change the mixing to actually use the retina positions?

@twolfson
Copy link
Owner

By default, we have lexicographical sorting to guarantee consistency in all situations. One common issue users have is something like ['icon.png', 'icon-variation.png'] and ['icon-2x.png', 'icon-variation-2x.png']. These wind up getting sorted differently because . is less than - but v is greater than 2:

> var normal = ['icon.png', 'icon-variation.png'];
undefined
> normal.sort();
[ 'icon-variation.png', 'icon.png' ]
> var retina = ['icon-2x.png', 'icon-variation-2x.png'];
undefined
> retina.sort();
[ 'icon-2x.png', 'icon-variation-2x.png' ]

That being said, we also support disabling sorting via algorithmOpts:

{
  // src, dest
  algorithmOpts: {
    sort: false
  }
}

@twolfson
Copy link
Owner

I forgot to write down the common solution to the naming issue. We recommend to use -1x consistently for normal sprites:

> var normal = ['icon-1x.png', 'icon-variation-1x.png'];
undefined
> normal.sort();
[ 'icon-1x.png', 'icon-variation-1x.png' ]
> var retina = ['icon-2x.png', 'icon-variation-2x.png'];
undefined
> retina.sort();
[ 'icon-2x.png', 'icon-variation-2x.png' ]

@Sebobo
Copy link
Author

Sebobo commented Jul 24, 2015

Ok thx for the good explanation, as I said I disabled the sorting in the algorithmOpts to solve my issue.

But if the -1x is your recommendation, then the examples should be adjusted, because it says: For example sprites/*.png should capture sprite1.png and sprite1-2x.png.
I mean this is something that can easily happen to a user and should be mentioned. I just want to help other people save some time :)

I was also confused, why in the generated sprites.scss there are all those variables for retina which are never used. Using those could make the output much more reliable no matter if I use sorting or not. even if it creates a bit more css.

@twolfson
Copy link
Owner

Ah, sorry. I misread your initial issue and found it confusing why you sad sort: false which explains a lot.

You are totally right, we should be showing sprite1-1x.png in the documentation. I think this issue has occurred enough times that it should be more easily avoided.

The excessive amount of variables in the non-CSS variation are due to the following reasoning:

  • If someone wants to use spritesmith as a plug and play, they are likely to use the .css variation.
  • If they have some custom build process, then they might want to do something custom which we can't predict. As a result, we expose everything we know in an easily consumable manner.

@Sebobo
Copy link
Author

Sebobo commented Jul 25, 2015

Ok cool thx. I think it would suffice to adapt the documentation and give a small hint regarding the naming.
I really like the possibilities of the scss in my projects 👍
It's just easy to miss some things in the chain of calls to the different functions.

@twolfson
Copy link
Owner

I started updating the documentation on this and am realizing the contradiction. Due to spritesmith's current setup, we assume the name of the non-retina sprite as the sprite's class (e.g. using sprite1-1x would yield a class of .sprite1-1x over .sprite1).

What about adding a note in the README about not allowing for variations that are sprite1 and sprite1-variation. Instead, we suggest either:

  • Always naming those variations in a similar fashion (e.g. sprite1-default)
  • Using an @2x in the name (as this will prevent conflicts with - and _)
> var normal = ['icon.png', 'icon-variation.png'];
undefined
> normal.sort();
[ 'icon-variation.png', 'icon.png' ]
> var retina = ['icon@2x.png', 'icon-variation@2x.png'];
undefined
> retina.sort();
[ 'icon-variation@2x.png', 'icon@2x.png' ]

@Sebobo
Copy link
Author

Sebobo commented Jul 28, 2015

Yes I like the idea with the "@2x" suffix because this is also kind of a standard now.
And maybe a short remake about the influence on sorting when naming files.

@twolfson
Copy link
Owner

I think I had a ignorant resistance to the @ character. I assumed it wouldn't be allowed on some OS' but I have successfully tried it out on both GNU/Linux and Windows. There is also a little frustration that @ must be encoded to %40 in URLs but technically it saves us more characters than -default.

I am going to update the documentation/corresponding files now. I am also considering adding in a safety check (e.g. if a retina sprite has a - in it aside from -2x, then prompt a warning but don't fail).

@twolfson
Copy link
Owner

Upon thinking about it further, the warning should really occur when an the a different amount of normal sprite names substring each other than retina sprite names substring each other.

// Example with a bad delimiter
> var a = ['hello', 'hellothere'];
undefined
> a.sort();
[ 'hello', 'hellothere' ]
> var b = ['helloz2x', 'hellotherez2x'];
undefined
> b.sort();
[ 'hellotherez2x', 'helloz2x' ]

But I think we are probably safe with the documentation for now.

@twolfson
Copy link
Owner

It turns out that if anyone was using the @2x syntax with any of our supported parsers (e.g. LESS, SASS, Stylus), then they would be bumping into variable issues due to @ being an invalid variable name character. Hooray for finding bugs 🎉

@twolfson
Copy link
Owner

We have released a patched version of grunt-spritesmith which supports @2x suffixes as well as has @2x usage for all sprites/documentation in 5.0.0. A similar release for gulp.spritesmith will be made shortly.

@Sebobo
Copy link
Author

Sebobo commented Jul 29, 2015

Awesome, great job!

I will try it in the next days.

@twolfson
Copy link
Owner

For the sake of our records, a similar release has been published in gulp.spritesmith@4.0.0.

@MarkiyanPyts
Copy link

MarkiyanPyts commented Aug 9, 2016

Guys what about issue with @ in SASS any update on that?

I am having errors because of it

Invalid CSS after "logo2": expected expression (e.g. 1px, bold), was "@2x: ("

@twolfson
Copy link
Owner

twolfson commented Aug 9, 2016

@DarthMarcius I don't believe that's related to this issue. Please open a separate issue

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

No branches or pull requests

3 participants