Skip to content
This repository has been archived by the owner on Aug 21, 2023. It is now read-only.

Swap grunt with gulp #18

Closed
wants to merge 9 commits into from
Closed

Swap grunt with gulp #18

wants to merge 9 commits into from

Conversation

stnvh
Copy link
Contributor

@stnvh stnvh commented Feb 4, 2015

Putting this here for the time being.
Current status is it works! (if you're not using nested media queries like so):

@incude respond('640px') {
    color: $green;

    @include respond('768px') {
        color: $blue;
    }
}

The reason some sites weren't compiling at all before was due to libsass' stricter type casting, which was formatting the output CSS as @media screen and (min-width: '640px') which is invalid (values aren't wrapped in quotes! look at _mixins diff).

EDIT:
I done some benchmarks:

Grunt (.sass-cache removed, *.css files removed):
1. 3.048s
2. 2.935s
3. 2.977s
--
avg: 2.987s

Gulp (cache n/a, *.css files removed):
1. 1.621s
2. 1.633s
3. 1.630s
--
avg: 1.628s

@kinglozzer
Copy link
Member

Does it work with this example?

.logo {
    background-image: 'foo.png';

    @include retina {
        background-image: 'foo@2x.png';
    }
}

Or putting a pixel-density query inside a max-width query (I don’t really fancy writing those out by hand!)? I think those two are the only times we really nest media queries.

There were quite a few other media query bugs that will be fixed in libsass 3.2 when it comes. gulp-sass uses node-sass v2.0.0-beta, which is using an even older version of libsass, so I’d be inclined to hold out a little longer until node-sass 2.0.0 stable arrives (in the mean time, we can just clone this branch if we need the speed boost).

@stnvh
Copy link
Contributor Author

stnvh commented Feb 4, 2015

The first example works correctly (and outputs the query at root level e.g normally):

@media only screen and (-webkit-min-device-pixel-ratio: 1.5), only screen and (min-device-pixel-ratio: 1.5), only screen and (min-resolution: 144dpi), only screen and (min-resolution: 1.5dppx) {
  .logo {
    background-image: url('foo@2x.png');
  }
}

Its only when queries are nested it will break, so the pixel density inside a max-width would break. If this is what you mean:

.logo {
    @include respond('1200px') {
        background-image: url('foo.png');

        @include retina(2) {
            background-image: url('foo@2x.png');
        }
    }
}

equates to:

@media screen and (min-width: 1200px) {
  .logo {
    background-image: url('foo.png');
  }@  media only screen and (-webkit-min-device-pixel-ratio: 2), only screen and (min-device-pixel-ratio: 2), only screen and (min-resolution: 192dpi), only screen and (min-resolution: 2dppx) {
    .logo {background-image: url('foo@2x.png');
  }
}}

As for waiting, I agree. It's more worth waiting for the new release, rather than forcing broken software into production for the sake of speed.

@stnvh
Copy link
Contributor Author

stnvh commented Feb 13, 2015

node-sass 2.0.x has been released, so now to wait for libsass 3.2 (4-6 weeks according to #812)

@stnvh
Copy link
Contributor Author

stnvh commented Mar 31, 2015

This branch now functions properly! Using node-sass@3.0.0-beta.4 with support for nested queries etc.

@feejin
Copy link
Member

feejin commented Mar 31, 2015

It won't let me merge automatically, has conflicts. Can you tag the current stable branch and then merge this yourself? Do we need to install extra things to run it (apart from the node-sass beta)? Is it safe to switch with node-sass still in beta?

@stnvh
Copy link
Contributor Author

stnvh commented Mar 31, 2015

You wont need to install anything, all done via dependencies! I'll tag n' merge on my machine, then push up. As for stability, it's fine, however it's not exactly good practice forcing a beta into production. The beta itself isn't broken/bleeding edge so it would work

@stnvh stnvh closed this in a7917ba Mar 31, 2015
@stnvh stnvh deleted the dev/gulp branch April 1, 2015 08:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants