Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve selectors parsing for nth-child binomials #1197

Merged
merged 1 commit into from
May 12, 2015

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented May 10, 2015

Fixes #593

Implementation may not be the cleanest, but maybe we find more use cases where "compress" can be applied to String Constants. Then we may get a better picture how we can implement it better.

@mgreter mgreter self-assigned this May 10, 2015
@mgreter mgreter added this to the 3.2.4 milestone May 10, 2015
@xzyfer
Copy link
Contributor

xzyfer commented May 11, 2015

My issue with this is that it's unclear when/if compressed should be used. It's another undocumented mystery flag that noone understands.

IMHO this issues isn't of any importance atm so we can defer it until 3.4 with the rest of the whitespace issues. By then we should have a better idea of if there are more usecases for compressed (I don't think there are from reading the Ruby code).

@mgreter
Copy link
Contributor Author

mgreter commented May 11, 2015

I somewhat agree, but I can explain quite clearly what that flag does. It removes all whitespace from the string if it is emitted to css in compressed mode. This is not really familiar with any other node. So not sure what to do with it. I thought about a specific ast node for binomials, but I still would need to preserve the full parsed string, since ruby sass really preserves the whitespace (it does not just add one optional space as other nodes). Or it would basically be another string type doing exactly what the new "compress" flag is doing ...

I somewhat disagree to postpone this. The result seems promising to be on par with ruby sass. I tend to search the code for these specific flags to see where they are used. So far this flag is really specific to binomials. As you see I pretty much fixed all other white space issues, so we probably can label the next bugfix release a "white-space" release, so I vote to include this.

Of course I'm always open to have the option named differently and/or add some description!

@xzyfer
Copy link
Contributor

xzyfer commented May 11, 2015

The concerns here are too mixed up IMO. Fixing an issue with binomials-like expressions in selectors by changing how we output strings?

At the very least the output logic should be scoped to selectors, at which point we could use context, rather then some mystery flag. This at least signals to the reader what the intent is.

@xzyfer
Copy link
Contributor

xzyfer commented May 11, 2015

Even renaming the flag to signal that it's about selectors, would be a big improvement.

@mgreter
Copy link
Contributor Author

mgreter commented May 11, 2015

Renamed the flag to can_compress_whitespace. I don't think it case much to do with selectors, I has specifically to do with those binomials. For me it even looks somewhat like a bug in compressed mode, which this flag reproduces (so we match ruby sass). But that is really just me guessing ...

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 80.13% when pulling af234df on mgreter:bugfix/issue_593 into fb33a2a on sass:master.

@xzyfer
Copy link
Contributor

xzyfer commented May 12, 2015

This is an improvement 👍

xzyfer added a commit that referenced this pull request May 12, 2015
Improve selectors parsing for nth-child binomials
@xzyfer xzyfer merged commit 1a3c30d into sass:master May 12, 2015
@mgreter mgreter deleted the bugfix/issue_593 branch July 28, 2015 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loss of spaces in interpolated selectors
3 participants