BlessCSS / bless

CSS Post-Processor
blesscss.com
MIT License
282 stars 60 forks source link

Fixed an issue with lengths being undefined. #45

Closed barcellano-envoy closed 9 years ago

barcellano-envoy commented 9 years ago

Also, it looks like this was added at some point to the compiled lib and then removed.

aabenoja commented 9 years ago

Added that comment to the wrong spot. My apologies.

barcellano-envoy commented 9 years ago

I believe the issue is in @keyframes, eg

@-webkit-keyframes sound-wave {
  0% {
    opacity: 0.6;
    -webkit-transform: scale3d(0, 0, 0);
    -moz-transform: scale3d(0, 0, 0);
    -ms-transform: scale3d(0, 0, 0);
    -o-transform: scale3d(0, 0, 0);
    transform: scale3d(0, 0, 0);
  }

  100% {
    opacity: 0;
    -webkit-transform: scale3d(1, 1, 1);
    -moz-transform: scale3d(1, 1, 1);
    -ms-transform: scale3d(1, 1, 1);
    -o-transform: scale3d(1, 1, 1);
    transform: scale3d(1, 1, 1);
  }
}

How do I add a test and run the tests?

aabenoja commented 9 years ago

Look at the test folder and you'll find two folders. The input folder contains css files that may or may not exceed the selector limit. The output folder contains folders corresponding to a matching input css filename. The output css files are 0.css, 1.css, 2.css depending on the length of the input css file for that test.

For these keyframes test I suggest modeling after the media queries test.

barcellano-envoy commented 9 years ago

I did create the css file for the test. How do I run the tests?

aabenoja commented 9 years ago

In a local clone with the changes I suggested I did not run into an undefined nestedRule.selectors, mainly because the keyframes at-rule does not contain any selectors. I tried an empty media query but the css parser module was smart enough to give nestedRule.selectors an empty array.

As far as testing is concerned run npm test in your console.

barcellano-envoy commented 9 years ago

Forgive my coffeescript, never really used it much. I have updated the code with your suggestion, and also added a test for the keyframe case

aabenoja commented 9 years ago

@paulyoung Got any input?

barcellano-envoy commented 9 years ago

Ok, I also figured out another test case where even this fails

 @media screen and (max-width: 1199px) {
    #selector {
        color: #fffff;
    }
    @media screen and (max-width: 991px) {
        #selector {
            color: #fffff;
        }
        @media screen and (max-width: 767px) {
            #selector {
                color: #fffff;
            }
        }
    }
}

where

for nestedRule in rule.rules
          numNestedRuleSelectors += nestedRule.selectors.length

nestedRule.selectors can be undefined.

What's the best coffeescript way to resolve this?

aabenoja commented 9 years ago

As much as I dislike nested media queries the current implementation will NOT hit that third media query. This is most likely also breaks if a rule contains a media query as well. Thanks for catching this ugly excuse for valid css.

barcellano-envoy commented 9 years ago

It's what libsass compiles the SASS we have to. :( Ruby sass does not.

barcellano-envoy commented 9 years ago

What do you think is the best solution? Recursive function to travel down the tree?

aabenoja commented 9 years ago

I believe a recursive function would be best in this case, yes. We'll need to apply it to both rules and media queries.

barcellano-envoy commented 9 years ago

I'm pretty naive on my coffeescript skills. Want me to give it a go, or would it be better to wait for someone closer to the project?

barcellano-envoy commented 9 years ago

Actually, according to the test suite, this makes the test pass

for nestedRule in rule.rules
          break if typeof nestedRule.selectors is 'undefined'

          numNestedRuleSelectors += nestedRule.selectors.length

Will commit for you to see

barcellano-envoy commented 9 years ago

Ahh, I see. It probably wouldn't break the file on the correct selector

aabenoja commented 9 years ago

I say give it a go. I'm not very coffeescript savvy myself; I personally prefer to write my javascript by hand. I have something in mind. I'll submit a PR to your fork in a bit.

barcellano-envoy commented 9 years ago

Ok, I'm going to write a few more tests.

aabenoja commented 9 years ago

Well, the css parser is blowing up trying to break apart nested media queries. Good to know.

aabenoja commented 9 years ago

So the css parser used in this project is blowing up trying to parse those nested media queries. It dies completely every time it tries on my end, at least. I suppose now is as good a time as ever to try PostCSS.

barcellano-envoy commented 9 years ago

The test I wrote passes. Not sure why, but it does. I'm sure it's a flaw with the test probably.

aabenoja commented 9 years ago

npm run build then npm test and tell me if it doesn't break.

Scratch that, I was still running off of modified node modules.

aabenoja commented 9 years ago

I must have had a bad character somewhere in my test file that caused the blowing up. Weird, but fine.

aabenoja commented 9 years ago

I'll be updating the unit test again shortly.

paulyoung commented 9 years ago

Wow, don't know how I missed this one for so long. I'll review when I have more time.

Thanks for the effort, and sorry for the silent treatment!

aabenoja commented 9 years ago

This is no longer valid with #63.