-
Notifications
You must be signed in to change notification settings - Fork 80
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
update less.js to 2.4.0 #79
base: master
Are you sure you want to change the base?
Conversation
It looks like all the testcases are failing. Wer you able to get them running on your machine? |
@cowboyd Yes. I also posed a pull request on the |
That PR, cowboyd/commonjs.rb#17 also fails, and is not safe to merge |
well, let me have an eye on both commits ... 2015-03-18 3:11 GMT+08:00 Charles Lowell [email protected]:
|
@ftiasch Have you any ideas how to fix CI problems in this and related PR? |
Sorry, I haven't. I got so busy these days to have no time the keep track 2015-04-03 16:20 GMT+08:00 Maxim Dobryakov [email protected]:
|
I've spend few hours on this today. I'll try to continue soon. I'm just not sure if we need to keep usage of |
The lessjs document says in new version (>= 2.0) we should use less.render 2015-04-28 5:18 GMT+08:00 Josef Šimánek [email protected]:
|
Sure, but |
Yes. But I think we'd better be consistent with what 2015-04-28 19:18 GMT+08:00 Josef Šimánek [email protected]:
|
sadly it looks like commonjs.rb doesn't support 2 things:
the first one is easy to fix but the second is a bit more challenging. |
@rstacruz I have made a pull request cowboyd/commonjs.rb#18 to fix these two (Thanks to @simi for adding tests). However, the pr seems not yet be merged. |
hmm, tests are failing on this one even with that PR being applied to common.js. |
|
Note: commonjs cowboyd/commonjs.rb#18 causes that library's test suite to fail which is why it has not yet been merged. |
Actually nevermind, looks like the specs are passing on that one, there is still a little bit of work to do there wrt test coverage. |
I'm still on this. But I'm not sure about my question in #79 (comment). |
@simi is there a problem supporting both? |
I don't see any benefits for that. |
Well, if we remove the old API, then it means that any dependent libraries need to be updated. This includes https://github.com/metaskills/less-rails for example. |
I need to add some words here: I also tried to update less-rails, but the old rails helper (asset-url, asset-path etc) seems not working and I cannot figure out why. So I didn't send a PR on less-rails. There may be some more work to be done. |
@ftiasch can you share your code? |
@simi It's on ftiasch/less-rails . trivial change |
@cowboyd I think we can keep public API same for both variants. |
Was bitten by the pre-2.x version of LESS in a project the other day, since desc "Compile LESS master to CSS output file"
task :compile_less do
puts "\n"
print "Compiling LESS... "
sh '/usr/local/bin/lessc assets/css/styles.less _site/assets/css/styles.less.css'
puts "done!"
puts "\n"
end # task :compile_less This feels gross to me. Does anybody have a better way to use Less 2.x? I'm a total Ruby / Jekyll novice and have no idea what I'm doing, but the above worked and let me add the Rake task to my Guardfile. |
Is there any enthusiasm for getting this moving again? |
close #77