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

Set line-comments by default like sass-rails #24

Merged
merged 1 commit into from
Jul 25, 2015

Conversation

edward
Copy link
Member

@edward edward commented Jun 30, 2015

Sets line_comments by default like sass-rails

Note that this depends on a patch version bump in sassc-ruby

@edward
Copy link
Member Author

edward commented Jun 30, 2015

For review @bolandrm

@edward edward changed the title Set line-comments by default like sass-rails Set line-comments by default like sass-rails Jun 30, 2015
@bolandrm
Copy link
Member

bolandrm commented Jul 2, 2015

@edward looks fine, but there's a test to fix up

@edward
Copy link
Member Author

edward commented Jul 2, 2015

@bolandrm it's failing because sassc-ruby needs an update. Should run green after my PR over there is merged and the test here re-run.

@bolandrm
Copy link
Member

@edward sorry, i didn't quite understand what you meant initially. I bumped the sassc-ruby version, we good to merge this now?

@edward
Copy link
Member Author

edward commented Jul 13, 2015

@bolandrm heyo – let me rebase onto master and I’ll get the PR ready to merge.

@bolandrm
Copy link
Member

@edward since i've added source maps, should we ensure that line comments get turned off when source maps get turned on? (I can do that, just asking)

@edward
Copy link
Member Author

edward commented Jul 13, 2015

@bolandrm Good question. I feel like libsass is usually smart enough to know which options to disable, e.g. if you turn on line comments but have a compressed style of output, it’ll ignore the line comments option.

Do you have time to write a test to see what the output looks like with both line comments and source maps?

@bolandrm
Copy link
Member

Yeah, i'll take a look

@edward
Copy link
Member Author

edward commented Jul 15, 2015

@bolandrm It looks like we get output that looks like this:

"/* line 3, /home/vagrant/src/sassc-rails/test/dummy/app/assets/stylesheets/application.scss */\n.hello {\n  color: #FFF;\n}\n\n/*# sourceMappingURL=data:application/json;base64,ewoJInZlcnNpb24iOiAzLAoJImZpbGUiOiAic2Fzc2MtcmFpbHMvdGVzdC9kdW1teS9hcHAvYXNzZXRzL3N0eWxlc2hlZXRzL2FwcGxpY2F0aW9uLmNzcyIsCgkic291cmNlcyI6IFsKCQkic2Fzc2MtcmFpbHMvdGVzdC9kdW1teS9hcHAvYXNzZXRzL3N0eWxlc2hlZXRzL2FwcGxpY2F0aW9uLnNjc3MiCgldLAoJInNvdXJjZXNDb250ZW50IjogWwoJCSIkYi1jb2xvcjogI0ZGRjtcblxuLmhlbGxvIHtcbiAgY29sb3I6ICRiLWNvbG9yO1xufVxuIgoJXSwKCSJtYXBwaW5ncyI6ICI7QUFFQSxNQUFNLENBQUM7RUFDTCxLQUFLLEVBSEcsSUFBSTtDQUVOIiwKCSJuYW1lcyI6IFtdCn0= */"

I’ve confirmed that at least Chrome 43.0.2357.132 is happy with this and is able to pick out the inlined sourcemap since those lines go at the end of imported file statements, and not on the same lines as the comments.

Seems like we’re good here?

bolandrm added a commit that referenced this pull request Jul 25, 2015
Set line-comments by default like sass-rails
@bolandrm bolandrm merged commit 1f48d55 into sass:master Jul 25, 2015
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.

2 participants