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

@mixin jquery-text-shadow outputs "false" for undefined shadows, breaking the text-shadow declaration #14

Open
isGabe opened this issue Jun 26, 2013 · 5 comments

Comments

@isGabe
Copy link

isGabe commented Jun 26, 2013

Using your example

@include jquery-text-shadow(1px 1px 2px rgba(255, 0, 0, 0.5), 3px 3px 2px rgba(0, 255, 0, 0.5), 5px 5px 2px rgba(0, 0, 255, 0.5), 7px 7px 2px rgba(0, 0, 0, 0.5));

I get the following CSS output, which does not work.

text-shadow: 1px 1px 2px rgba(255, 0, 0, 0.5), 3px 3px 2px rgba(0, 255, 0, 0.5), 5px 5px 2px rgba(0, 0, 255, 0.5), 7px 7px 2px rgba(0, 0, 0, 0.5), false, false, false, false, false, false

The mixin seems to be outputting the false arguments for any of the 10 $shadow- variables left undefined:

@mixin jquery-text-shadow($shadow-1: default, $shadow-2: false, $shadow-3: false, $shadow-4: false, $shadow-5: false, $shadow-6: false, $shadow-7: false, $shadow-8: false, $shadow-9: false, $shadow-10: false) {

SASS docs show that multiple variable arguments can be defined like this:

@mixin text-shadow($shadows...) {text-shadow: $shadows}

so that:
@include text-shadow(0px 4px 5px #666, 2px 6px 10px #999);

will output to:
`text-shadow: 0px 4px 5px #666, 2px 6px 10px #999;

But your mixin is much more complex, so I'm not sure if that would work. In any case I'm wondering if it's a Compass/SASS version issue.

I am running SASS v3.2.7 and Compass v0.13.alpha.4

@heygrady
Copy link
Owner

Sounds like a breaking change in SASS recently. I'll take a look at it. I certainly wasn't seeing that when I last made a commit to this project.

@isGabe
Copy link
Author

isGabe commented Jun 26, 2013

Yeah, I figured it was probably an issue with the SASS or Compass version I'm using. Some Compass plugins I use require the latest versions...too bad they make breaking changes, but that's how it goes, I guess.

Small price to pay for amazing open source tools.

@isGabe
Copy link
Author

isGabe commented Jun 26, 2013

So just to see if it was an easy fix I tried editing the mixin to use the ... argument syntax, and it worked for me on the first try. Normal text shadow showed up as expected, and it worked in IE9 as well. Not sure if you want a pull request or not, since I haven't tested it on previous versions of SASS/Compass, or other versions of IE yet, but this is what I did:

Lines 31 - 38 of _jquery.textshadow.scss:

31 @mixin jquery-text-shadow($shadow...) {
32 // for supported browers
33 @include text-shadow($shadow);
34
35 @if $shadow != "" {
36    $shadow: $default-text-shadow-color $default-text-shadow-h-offset $default-text-shadow-v-offset $default-text-shadow-blur;
37 }
38 $shadows: ($shadow);

I'll proceed with implementing it in the project I'm working on and see if I run into anything else.

heygrady added a commit that referenced this issue Jun 26, 2013
…dows before using compass' text-shadow function.
@heygrady
Copy link
Owner

See if this fixes it for you.

@heygrady
Copy link
Owner

The reason I didn't switch it completely to the new ... syntax is that compass 0.12.x does it the old way:

stable 0.12.x

master 0.13.x

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

2 participants