Golden-Grid-System icon indicating copy to clipboard operation
Golden-Grid-System copied to clipboard

Use of Sass's @extend directive inside media queries leads to sadness.

Open mysterycommand opened this issue 14 years ago • 4 comments

Hi, I just wanted to point out that in the Sass version of the grid, the @extend directive is used inside "IE fixes" and the min-width: 40em media query. Because of the way Sass handles @extend this results in the .huge declaration being converted to:

.huge, .ie h1, h1 {
  /* 42px / 48px  */
  font-size: 2.625em;
  line-height: 1.143em;
}

… which I don't think is what's intended. Less has a slightly simpler mixin syntax, but I think this is what you want:

@mixin huge {
    /* 42px / 48px  */
    font-size: #{42 / $em}em;
    line-height: ($line *2) / 42  * 1em;
}
.huge { @include huge; }

… then, in the IE-specific, and media query declarations, you'd use the same, @include huge; … this produces a CSS document that looks more like the GGS.css that's included in the source.

mysterycommand avatar Sep 12 '11 20:09 mysterycommand

Thanks for pointing this out!

I don't currently use SASS myself, so I can't test this out properly. I'll make a note to convert all the "LESS mixins" into proper SCSS mixins when I get the chance to do so.

jonikorpi avatar Sep 23 '11 13:09 jonikorpi

Mixins lead to duplicated code easily, while @extend directives do it all in a DRY approach. It's better for performance, file size and maintenance.

The compiled CSS will be different, but I think it would be better if GGS used the best features of each preprocessor.

Example:

SASS with mixins (@include and @mixin):

/* Source */
@mixin huge {
    font-size: 100px;
}

.ie h1 {
     @include huge;
}


/* Compiled CSS */
huge {
    font-size: 100px;
}

.ie h1 {
    font-size: 100px;
}

SASS with @extend directives:

/* Source */
.huge {
    font-size: 100px;
}

.ie h1 {
    @extend .huge
}


/* Compiled CSS */
.huge, .ie h1 {
    font-size: 100px
}

As you can see, in this example the filesize is almost insignificant, but in real CSS files mixins compile to a lot bigger and unperformant files.

frankpf avatar Sep 23 '11 15:09 frankpf

Yeah, using mixing produces more code … more CSS declarations, that's true. I was simply pointing out that this in Less:

.huge {
    /* 42px / 48px */
    font-size: 42 / @em;
    line-height: (@line*2) / 42 * 1em;
}
/* ... */
.ie h1 {
    .huge();
    margin: (48/42*1em) 0 (24/42*1em);
}

… is equivalent to this in Sass:

@mixin huge {
    /* 42px / 48px  */
    font-size: #{42 / $em}em;
    line-height: ($line *2) / 42  * 1em;
}
/* ... */
.ie h1 {
    @include huge;
    margin: (48/42*1em) 0 (24/42*1em);
}

… I know mixins produce more code, but they also produce different code than @extend, and in this particular case, using @extend .huge; inside a media query pulls the h1 declaration out of the media query … which could have unexpected/unwanted consequences.

mysterycommand avatar Sep 23 '11 15:09 mysterycommand

With sass you don't HAVE to compile mixins to CSS. The sass implementation of GGS should convert all of the classes like .huge, .massive, .wrapper etc. to mixins in a partial. If you aren't going to use those classes in your markup there is no need to define them as classes.

A simple example would be to create a directory called "partials" add a file called _type-presets.sass (or scss) where you define the type preset mixins.


=huge 
    /* 42px / 48px  */
    font-size: #{42 / $em}em
    line-height: ($line *2) / 42  * 1em
...

Then you can @import "partials/type-presets"

and use

#MyOwnMarkup
    +huge

Then your actual CSS file won't have a bunch of unused classes.

Maybe it's out of scope for this project or repo but the current GGS.scss file, although helpful and appreciated, isn't that much of a value add as it doesn't really leverage sass/scss as much as it could. It would be nice to have something like the compass-less-plugin for GGS. I will try to get the ball rolling on that and do it without making it a compass plugin (just sass/scss).

xtalx avatar Oct 03 '11 21:10 xtalx