Igosuki / compass-mixins

A collection of compass' stylesheet for bower dependencies and libsass
Other
592 stars 197 forks source link

compact() removed from libsass 3.0.1 #34

Closed kjbbb closed 8 years ago

kjbbb commented 9 years ago

A recent change in libsass broke compass compatibility by removing the compact() function... The function was removed because it's not in the sass spec.

https://github.com/sass/libsass/commit/003d3dce28d5a9296339cd8da8b051cce525c5cc

Zauberfisch commented 9 years ago

I hot fixed this issue by adding this function:

@function compact($var-1, $var-2: false,
$var-3: false, $var-4: false,
$var-5: false, $var-6: false,
$var-7: false, $var-8: false,
$var-9: false, $var-10: false) {
    $full: $var-1;
    $vars: $var-2, $var-3, $var-4, $var-5,
    $var-6, $var-7, $var-8, $var-9, $var-10;

    @each $var in $vars {
        @if $var {
            $full: $full, $var;
        }
    }
    @return $full;
}

taken from http://stackoverflow.com/a/11752227

Zauberfisch commented 9 years ago

I have to correct my earlier statement. It did not solve all problems, While I was no longer getting errors, I noticed that the css output was no longer valid. With the above function I was getting things like:

/* from @include transition(opacity 3000ms ease); it generated: */
-webkit-transition: opacity false false, 3000ms false false, ease false false;
-moz-transition: opacity false false false, 3000ms false false false, ease false false false;
-o-transition: opacity false false false, 3000ms false false false, ease false false false;
transition: opacity 3000ms ease;

I have written my own compact function now, which seems to solve the problem (though its pretty late, so I might be missing something). It loops the list recursively to ensure all false-ish values are removed.

@function _compact($var) {
    @if type_of($var) == 'list' {
        $full: ();
        @each $item in $var {
            @if $item {
                $full: append($full, _compact($item));
            }
        }
        @return $full;
    }
    @return $var;
}

@function compact($var-1, $var-2: false, $var-3: false, $var-4: false, $var-5: false, $var-6: false, $var-7: false, $var-8: false, $var-9: false, $var-10: false) {
    @return _compact(($var-1, $var-2, $var-3, $var-4, $var-5, $var-6, $var-7, $var-8, $var-9, $var-10));
}

PS: please note that there also has been bug (https://github.com/sass/libsass/issues/254) reported recently about false-ish values that are not evaluated as false which might effect the outcome of this function. that might cause issues with this code, but does not seem to be the case/has been fixed in latest master.

mikob commented 9 years ago

Both of those were giving me issues by not putting commas between multiple box-shadow properties. This works for me:

@function compact($vars...) {
    $list: ();
    @each $var in $vars {
        @if $var {
            $list: append($list, $var, comma);
        }
    }
    @return $list;
}
elsigh commented 9 years ago

I had this issue as well.

michaek commented 9 years ago

I saw that in the release notes, and thought of this as well. I think the functions above may make sense to include to resolve the problems. Is there a PR?

mikob commented 9 years ago

@michaek I never made one.

Igosuki commented 9 years ago

I'm adding @mikob's function to the new release.

adalinesimonian commented 9 years ago

@Igosuki +1 That's great news! I've been relying on a separate _compact.scss and it'd be great to finally dump that li'l old thing. :smiley:

Igosuki commented 9 years ago

Done in v0.12.7

bmiller59 commented 9 years ago

I had an issue with recursive compact arguments. It was leaving false items in. I combined the above solutions and this fixed it:

@if not(function-exists(compact)) {
  @function compact($vars...) {
    @each $var in $vars {
      @if type_of($var) == 'list' {
        $full: ();
        @each $item in $var {
          @if $item {
            $full: append($full, compact($item), comma);
          }
        }
        @return $full;
      }
      @return $var;
    }
    @return $vars;
  }
}
Igosuki commented 9 years ago

So this is a bug with the current version (0.12.7) ?

mikko-h commented 9 years ago

Yes, it is a bug in 0.12.7. mikob's implementation used in 0.12.7 leaves extraneous falses to the output like: -moz-transition: opacity 0.2s false false; However bmiller59's implementation has also a bug, it generates an extra comma when it is not needed, like: -moz-transition: opacity, 0.2s; Correct output should be: -moz-transition: opacity 0.2s;

EDIT: This works for me:

@function compact($vars...) {
    $first: nth($vars, 1);
    $sep: comma;
    $list: ();
    @if length($vars) == 1 and type_of($first) == 'list' {
        $vars: $first;
        $sep: list-separator($vars);
    }
    @each $var in $vars {
        @if $var {
            $list: append($list, $var, $sep);
        }
    }
    @return $list;
}

I got inspiration from the original ruby implemenation: https://github.com/Compass/compass/blob/master/core/lib/compass/core/sass_extensions/functions/lists.rb#L17

Igosuki commented 9 years ago

Closely implementing the ruby function is how it should be done in the first place, thanks.

thejmazz commented 9 years ago

Wish I came across this sooner..have spent the past hour or so pulling my hair over the fact that my simple @include box-shadow was not working. So far @mikko-h's solution has worked for me.

bjudson commented 9 years ago

I'm still getting incorrect output with version 0.12.7. For example the Sass: +transition(all 0.25s ease-in)

Yields:

-webkit-transition: all;
-moz-transition: all;
-o-transition: all;
transition: all;

I get the same output when I replace compact() with @mikko-h's solution.

leochen1216 commented 9 years ago

Same here. I'm using 0.12.7.

@include transition(max-height .2s);

output:

 -webkit-transition: max-height;
  -moz-transition: max-height;
  -o-transition: max-height;
  transition: max-height;
leochen1216 commented 9 years ago

This version (https://github.com/Igosuki/compass-mixins/pull/67) works for both single & multiple transitions:

@function compact($args...) {
    $first: nth($args, 1);
    $sep: comma;
    $list: ();

    @if length($args) == 1 and type_of($first) == 'list' {
      $args: $first;
      $sep: list-separator($args);
    }

    @for $index from 1 through length($args) {
      $arg: nth($args, $index);

      @if $arg {
        $list: append($list, $arg, $sep);
      }
    }

    @return $list;
  }
iampuma commented 8 years ago

Had the same problem with @include background(#ccc);

Above solution of leochen1216 works.

acha5066 commented 8 years ago

Can confirm that leochen1216 also fixed my issue with @include background(#ccc). Excuse my ignorance but why was this function deprecated in a minor release?

linibou commented 8 years ago

leochen1216 function's works perfectly for me, thank you ! issue with @include single-transition(border, .2s, ease-in);

xzyfer commented 8 years ago

Fixed in v0.12.8

halfnibble commented 7 years ago

How is this fixed, exactly?

Shouldn't the reimplementation of compact() be in "shared" or some other file that all scripts source?

afoeder commented 7 years ago

I am also troubled by the lack of the compact() function... functions/_lists.scss says on line 7,

// compact is part of libsass

but when googling for that it seems it has somehow removed from libsass? What's the state of this now?

Thanks!