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

compact() removed from libsass 3.0.1 #34

Closed
kjbbb opened this issue Feb 20, 2015 · 23 comments
Closed

compact() removed from libsass 3.0.1 #34

kjbbb opened this issue Feb 20, 2015 · 23 comments

Comments

@kjbbb
Copy link

kjbbb commented Feb 20, 2015

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.

sass/libsass@003d3dc

@Zauberfisch
Copy link

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
Copy link

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 (sass/libsass#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
Copy link

mikob commented Mar 22, 2015

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
Copy link

elsigh commented Apr 2, 2015

I had this issue as well.

@michaek
Copy link
Collaborator

michaek commented May 7, 2015

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
Copy link

mikob commented May 8, 2015

@michaek I never made one.

@Igosuki
Copy link
Owner

Igosuki commented May 9, 2015

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

@adalinesimonian
Copy link

@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. 😃

@Igosuki
Copy link
Owner

Igosuki commented May 10, 2015

Done in v0.12.7

@Igosuki Igosuki closed this as completed May 10, 2015
@bmiller59
Copy link

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
Copy link
Owner

Igosuki commented May 20, 2015

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

@mikko-h
Copy link

mikko-h commented May 21, 2015

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
Copy link
Owner

Igosuki commented May 23, 2015

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

@thejmazz
Copy link

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
Copy link

bjudson commented Sep 25, 2015

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
Copy link

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
Copy link

This version (#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
Copy link

iampuma commented Nov 4, 2015

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

Above solution of leochen1216 works.

@acha5066
Copy link

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
Copy link

linibou commented Feb 15, 2016

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

tobyj added a commit to tobyj/compass-mixins that referenced this issue Apr 8, 2016
@each $arg in $args won't work here when the first argument is list. It only returns first value in the first argument (list). 
eg: 
((opacity .2s ease), false, false, false)

The $arg will be opacity if we use @each, and this caused all the problems people mentioned in this issue: Igosuki#34

Tested with single transition and multiple transitions, all work fine.

// single transition
@include transition(opacity .2s ease);

// multiple transitions
@include transition(transform .2s ease-out, top .3s ease-out);
alinefm pushed a commit to kimchi-project/wok that referenced this issue Apr 13, 2016
More info: Igosuki/compass-mixins#34

Signed-off-by: Samuel Guimarães <sguimaraes943@gmail.com>
@xzyfer
Copy link
Collaborator

xzyfer commented May 19, 2016

Fixed in v0.12.8

@xzyfer xzyfer closed this as completed May 19, 2016
@halfnibble
Copy link

How is this fixed, exactly?

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

@afoeder
Copy link

afoeder commented Dec 17, 2016

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!

patchew-importer pushed a commit to patchew-project/kimchi that referenced this issue May 25, 2018
More info: Igosuki/compass-mixins#34

Signed-off-by: Samuel Guimarães <sguimaraes943@gmail.com>
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