Fix for SCSS debug info #148

Closed
qianc opened this Issue Aug 2, 2012 · 10 comments

Comments

Projects
None yet
8 participants
@qianc

qianc commented Aug 2, 2012

When including debug info in files created with Compass/SCSS, Respond.js won't work. An easy fix is to add "(?![\s]*-)" to the regex on line 109.

Before:

var qs = styles.match(  /@media[^\{]+\{([^\{\}]*\{[^\}\{]*\})+/gi )

After:

var qs = styles.match(  /@media(?![\s]*-)[^\{]+\{([^\{\}]*\{[^\}\{]*\})+/gi ),

The skips media queries of the following type:

@media -sass-debug-info{filename{font-family:<filename>}line{font-family:<line>}}
@paulirish

This comment has been minimized.

Show comment
Hide comment
@paulirish

paulirish Aug 2, 2012

Collaborator

tbh it seems kind of inappropriate for respond to handle the nasty debuginfo annotations.

Collaborator

paulirish commented Aug 2, 2012

tbh it seems kind of inappropriate for respond to handle the nasty debuginfo annotations.

@marcvangend

This comment has been minimized.

Show comment
Hide comment
@marcvangend

marcvangend Feb 21, 2013

Paul, I understand your point of view, but it cost me hours to find out that this was breaking respond.js. I accept debugging as part of my job, but it would be great if we can prevent other people from running into the same trouble.

Paul, I understand your point of view, but it cost me hours to find out that this was breaking respond.js. I accept debugging as part of my job, but it would be great if we can prevent other people from running into the same trouble.

@flokli

This comment has been minimized.

Show comment
Hide comment
@flokli

flokli Feb 27, 2013

Nope. At least in IE8, the "fix" from above doesn't work.

flokli commented Feb 27, 2013

Nope. At least in IE8, the "fix" from above doesn't work.

@paulirish

This comment has been minimized.

Show comment
Hide comment
@paulirish

paulirish Feb 27, 2013

Collaborator

@marcvangend yeah i agree with my. my earlier comment was.. wrong. :)

Collaborator

paulirish commented Feb 27, 2013

@marcvangend yeah i agree with my. my earlier comment was.. wrong. :)

@penx

This comment has been minimized.

Show comment
Hide comment
@penx

penx Jun 24, 2013

agreed, please fix this, I also just spent an hour or so debugging respond before finding out the issue is with the scss debug output.

penx commented Jun 24, 2013

agreed, please fix this, I also just spent an hour or so debugging respond before finding out the issue is with the scss debug output.

@scottjehl

This comment has been minimized.

Show comment
Hide comment
@scottjehl

scottjehl Jun 24, 2013

Owner

Would it be adequate to update the readme with a clear warning of potential conflicts with scss debug output? Working around it seems excessive for this script, but being clear about it up-front seems responsible and helpful.

Thoughts?

On Jun 24, 2013, at 5:21 AM, penx notifications@github.com wrote:

agreed, please fix this, I also just spent an hour or so debugging respond before finding out the issue is with the scss debug output.


Reply to this email directly or view it on GitHub.

Owner

scottjehl commented Jun 24, 2013

Would it be adequate to update the readme with a clear warning of potential conflicts with scss debug output? Working around it seems excessive for this script, but being clear about it up-front seems responsible and helpful.

Thoughts?

On Jun 24, 2013, at 5:21 AM, penx notifications@github.com wrote:

agreed, please fix this, I also just spent an hour or so debugging respond before finding out the issue is with the scss debug output.


Reply to this email directly or view it on GitHub.

@marcvangend

This comment has been minimized.

Show comment
Hide comment
@marcvangend

marcvangend Jun 26, 2013

If there is no acceptable workaround or fix, adding a clear warning to the readme is the least we should do, IMO.

That said, I'm not sure if I would call that solution "adequate". Let's face it: people don't read readme's in advance, but only when something breaks. If you enable respond.js and it doesn't work immediately, it's a no-brainer to check the readme. But suppose that respond.js is already working, you enable the SASS debug info, and the next day you discover that your responsive design breaks in IE8... Then checking the respond.js readme is not the first thing that comes to mind.

If there is no acceptable workaround or fix, adding a clear warning to the readme is the least we should do, IMO.

That said, I'm not sure if I would call that solution "adequate". Let's face it: people don't read readme's in advance, but only when something breaks. If you enable respond.js and it doesn't work immediately, it's a no-brainer to check the readme. But suppose that respond.js is already working, you enable the SASS debug info, and the next day you discover that your responsive design breaks in IE8... Then checking the respond.js readme is not the first thing that comes to mind.

@scottjehl

This comment has been minimized.

Show comment
Hide comment
@scottjehl

scottjehl Jun 26, 2013

Owner

My concern is working around an edge case that is generated only when debugging Sass. Complicating the source code to work around an issue that won't likely occur in production environments (or non-sass environements) seems less than ideal. If we could update the readme that'd be a great first step. Anyone able to help on this?

On Jun 26, 2013, at 10:43 AM, marcvangend notifications@github.com wrote:

If there is no acceptable workaround or fix, adding a clear warning to the readme is the least we should do, IMO.

That said, I'm not sure if I would call that solution "adequate". Let's face it: people don't read readme's in advance, but only when something breaks. If you enable respond.js and it doesn't work immediately, it's a no-brainer to check the readme. But suppose that respond.js is already working, you enable the SASS debug info, and the next day you discover that your responsive design breaks in IE8... Then checking the respond.js readme is not the first thing that comes to mind.


Reply to this email directly or view it on GitHub.

Owner

scottjehl commented Jun 26, 2013

My concern is working around an edge case that is generated only when debugging Sass. Complicating the source code to work around an issue that won't likely occur in production environments (or non-sass environements) seems less than ideal. If we could update the readme that'd be a great first step. Anyone able to help on this?

On Jun 26, 2013, at 10:43 AM, marcvangend notifications@github.com wrote:

If there is no acceptable workaround or fix, adding a clear warning to the readme is the least we should do, IMO.

That said, I'm not sure if I would call that solution "adequate". Let's face it: people don't read readme's in advance, but only when something breaks. If you enable respond.js and it doesn't work immediately, it's a no-brainer to check the readme. But suppose that respond.js is already working, you enable the SASS debug info, and the next day you discover that your responsive design breaks in IE8... Then checking the respond.js readme is not the first thing that comes to mind.


Reply to this email directly or view it on GitHub.

@Darep

This comment has been minimized.

Show comment
Hide comment
@Darep

Darep Aug 15, 2013

Contributor

Just ran into this problem myself, also. Spent about an hour, not understanding what the hell is going on. The moment I looked at the generated CSS I figured it was the the source maps with their funky @media -sass-debug-info syntax :D This should definitely be documented in the readme, at the very least. I'll probably create a PR.

I can understand the point about not wanting to over-complicate the source code, or mess with a working system. (If it aint' broken, etc.) But it's lame that I have to disable source maps and restart Rails everytime I want to test IE, making IE testing even more cumbersome. Boo. Non-code-changing solution would be nice.

PS. Respond.js rocks and you're awesome ❤️

Contributor

Darep commented Aug 15, 2013

Just ran into this problem myself, also. Spent about an hour, not understanding what the hell is going on. The moment I looked at the generated CSS I figured it was the the source maps with their funky @media -sass-debug-info syntax :D This should definitely be documented in the readme, at the very least. I'll probably create a PR.

I can understand the point about not wanting to over-complicate the source code, or mess with a working system. (If it aint' broken, etc.) But it's lame that I have to disable source maps and restart Rails everytime I want to test IE, making IE testing even more cumbersome. Boo. Non-code-changing solution would be nice.

PS. Respond.js rocks and you're awesome ❤️

@jefflembeck

This comment has been minimized.

Show comment
Hide comment
@jefflembeck

jefflembeck Nov 20, 2013

Collaborator

Pulled the README related piece in here. A code-changing solution might be a solution for the deep future, but not currently.

Collaborator

jefflembeck commented Nov 20, 2013

Pulled the README related piece in here. A code-changing solution might be a solution for the deep future, but not currently.

stevenyxu added a commit to stevenyxu/Respond that referenced this issue Feb 21, 2014

Added gotcha remarks on default yeoman webapp generator producing sas…
…s debug blocks breaking respond.js.

This hopefully helps out developers who have sass debug blocks enabled out of the box without explicitly enabling them (so that they might not recognize this caveat line when they first see it). The related discussion was in #148.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment