-
Notifications
You must be signed in to change notification settings - Fork 193
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
Use SassC to compile Sass #2176
Conversation
d63564a
to
df8747e
Compare
Relies on a fork at the moment, hoping to have the change merged upstream: sass/sassc-rails#17
df8747e
to
0999648
Compare
5a243df
to
8f6b454
Compare
end | ||
end | ||
|
||
Sprockets::Base.send(:prepend, Extensions::Sprockets::Engines) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit gross, but I assume there's no better way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should have mentioned, this is the current recommended workaround from sass/sassc-rails#6 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth a comment, or at least adding to the commit message?
👍 |
@@ -67,7 +67,8 @@ else | |||
end | |||
|
|||
gem 'sass', '3.4.9' | |||
gem 'sass-rails' | |||
gem 'sass-rails' # required by bootstrap-sass, but not listed as a dependency of it | |||
gem 'sassc-rails', github: 'bolandrm/sassc-rails', branch: 'master' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgive my possible misunderstanding on how Gemfiles and Gemfile.locks work but if the master branch changes on this repo is there a chance that we will get that when we bundle? Would it be safer to point this at a sha rather than a branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot @edds.
We'd have to bundle update
to change the version, and the lock file would change and need to be checked in to take effect. In theory that means both the committer and the reviewer would be encouraged to ask "why is this?", but it would definitely be better to specify a tag or a sha here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, hopefully this is a short-term thing, the gem should be released at some point soon at which point we can point it at a release.
Relies on a fork at the moment, hoping to have the change merged upstream:sassc-rails
Gem not been released yet, relies on a bugfix in master only: sass/sassc-rails#17I've compared the output of both engines in
:expanded
mode. It's not the easiest diff in the world to review, but it does seem to be mostly whitespace, wrapping of long selectors and leading zeroes, e.g..5em
versus0.5em
: https://gist.github.com/boffbowsh/28da0cdd02022b26eada.<
is ruby Sass,>
is SassC.Speedup on my laptop is 47s versus 3m11s.