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

Adding back Sass compressor #1626

Closed

Conversation

guilleiguaran
Copy link
Member

Adding back support for sass-rails CSS compressor with config.assets.css_compressor = :scss

Fix #1578 and #1619

cc @chriseppstein: please, can you review this? :)

@dmathieu
Copy link
Contributor

Could you not add the empty spaces line 33 please ?

@dmathieu
Copy link
Contributor

And add a test ?

@guilleiguaran
Copy link
Member Author

Actually I was searching but I don't found any test for compressors, I think I can add some tests for entire file in this or in another pull request

In Line 33 I remove trailing whitespaces :)

@chriseppstein
Copy link
Contributor

This hardcoding approach was removed for a reason. Sprockets needs to provide a public api for adding shorthands for compressors. Something like:

Sprockets.register_css_compressor(:scss, Sass::Rails::CssCompressor.new)

@chriseppstein
Copy link
Contributor

But that would only be for other projects to use. The sass-rails plugin automatically sets this up for you. You're best bet is to remove this line altogether.

@wycats: I recommend this pull request be closed and not merged.

@guilleiguaran
Copy link
Member Author

@chriseppstein, I was thinking the same, it needs to provide a public api to register javascript and css compressors, I will work on it.

Then, adding sass-rails plugin to the project fix the error described in #1578 and #1619, right?

@chriseppstein
Copy link
Contributor

@guilleiguaran It's unnecessary configuration. Sass-rails does this automatically. Just remove the setting.

@guilleiguaran
Copy link
Member Author

Thanks!!!, I'm closing this

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.

None yet

3 participants