MediaQuery.text minor bug #30

Merged
merged 3 commits into from Nov 16, 2012

Conversation

Projects
None yet
3 participants
Contributor

mirceapiturca commented Aug 23, 2012

MediaQuery mediaType and features needs to be joined trough "and".

For example parsing "only screen and (min-width:450px) and (max-width:1950px)", the text output will be
"only screen (min-width:450px) and (max-width:1950px)".
It is parsed right but the new text string will be invalid if matched with window.matchMedia

Thank you.

Update build/node-parserlib.js
MediaQuery mediaType and features needs to be joined trough "and".
Not a big issue but window.matchMedia reports the MediaQuery .text as invalid 

This pull request passes (merged 74592d6 into 63fd3bf).

Contributor

nzakas commented Aug 23, 2012

Thanks for the pull request, but this isn't the right location for such a fix. You actually need to make a change in the source file not in the completely built file.

Contributor

mirceapiturca commented Aug 23, 2012

Sorry about this. Indeed I see that the right file is parser-lib /src /css/MediaQuery.js. Should I do another pull request with the right file?

Thank for this wonderful piece of work!

Contributor

nzakas commented Aug 23, 2012

Yep, that's the right place to do it. If you could also add in some unit tests that verify this new functionality, I would be very appreciative.

Update src/css/MediaQuery.js
MediaQuery mediaType and features needs to be joined trough "and".
For example parsing "only screen and (min-width:450px) and (max-width:1950px)", the text output will be
"only screen (min-width:450px) and (max-width:1950px)".
It is parsed right but the new text string will be invalid if matched with window.matchMedia.

This pull request passes (merged 081dddb into 63fd3bf).

Contributor

mirceapiturca commented Aug 24, 2012

Added the change at the right file src/css/MediaQuery.js and set up a small test using the modified parser functionality at: http://typefolly.com/nzakas-parser-lib-63fd3bf/tests/css/CSSMediaQueryTests.html.
Not sure if this is the right way to do it.
Thank you.

Contributor

nzakas commented Sep 10, 2012

Sorry for the delay, I was traveling. The test should be done as a standalone JavaScript file kind of like this: https://github.com/nzakas/parser-lib/blob/master/tests/css/Parser.js

That way, it automatically gets picked up by the build and run.

Contributor

mirceapiturca commented Sep 11, 2012

Sorry, I have not done unit testing. So basically all I have to do is to add my tests to the Parser.js file from my forked version? Thanks.

Contributor

nzakas commented Sep 11, 2012

Yup

Update tests/css/Parser.js
Added test for MediaQuery.text.
Contributor

mirceapiturca commented Sep 12, 2012

I've appended a MediaQuery.text test onto the existing tests. Hope it will work.

Contributor

nzakas commented Oct 15, 2012

It looks like the tests are failing (according to the Travis build - sorry about it not turning red, I need to fix that).

nzakas added a commit that referenced this pull request Nov 16, 2012

@nzakas nzakas merged commit 172a2b2 into CSSLint:master Nov 16, 2012

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment