Skip to content

Conversation

nacin
Copy link
Contributor

@nacin nacin commented Oct 6, 2014

This (note whitespace around max-width: 710px) should be valid:

@media screen and ( max-width: 710px ) {}

Currently the whole line errors out:

[L1:C20]
>> ERROR: Expected IDENT at line 1, col 20. This rule looks for recoverable syntax errors. (errors) Browsers: All
[L1:C32]
>> ERROR: Expected LBRACE at line 1, col 32. This rule looks for recoverable syntax errors. (errors) Browsers: All
[L1:C32]
>> ERROR: Unexpected token '710px' at line 1, col 32. This rule looks for recoverable syntax errors. (errors) Browsers: All
[L1:C38]
>> ERROR: Unexpected token ')' at line 1, col 38. This rule looks for recoverable syntax errors. (errors) Browsers: All
[L1:C40]
>> ERROR: Unexpected token '{' at line 1, col 40. This rule looks for recoverable syntax errors. (errors) Browsers: All
[L1:C41]
>> ERROR: Unexpected token '}' at line 1, col 41. This rule looks for recoverable syntax errors. (errors) Browsers: All

This PR fixes CSSLint/csslint#541 with a simple this._readWhitespace(); call, and includes tests.

testMediaWithSimpleExpressionWithSpaces: function(){
var parser = new Parser({ strict: true});
var result = parser.parse("@media ( max-width:400px ) { }");
Assert.isTrue(true); //just don't want an error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way to assert something more meaningful? Could you test error count?

@stubbornella
Copy link
Member

Looks good overall. Just a couple questions about the tests.

@nacin nacin force-pushed the media-feature-spaces branch from ef8d866 to 88f40eb Compare February 14, 2015 18:18
@nacin
Copy link
Contributor Author

nacin commented Feb 14, 2015

@stubbornella Sure, these assertions are certainly suspect. Please note this was me just following the existing pattern in the file. I added two Assert.isTrue(true); — the related tests already had 15 just like it, for the same purposes, dating to 2011. (21c91f8)

So, yes, all of these should probably be refactored, but probably in a separate PR, and probably by someone with a bit more knowledge of parser-lib. 😅

nschonni added a commit that referenced this pull request Mar 2, 2015
Allow spaces within media query features
@nschonni nschonni merged commit 936f5b8 into CSSLint:master Mar 2, 2015
@nschonni nschonni added this to the v0.2.6 milestone Mar 2, 2015
@nacin nacin deleted the media-feature-spaces branch March 4, 2015 07:01
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

Successfully merging this pull request may close these issues.

Blank space within media query param throws 6 errors!
3 participants