Drop grunt-recess for grunt-contrib-less #11790

mdo · 2013-12-09T03:13:31Z

This is the first part to migrating away from Recess and to something that's more up to date with LESS. It adds a few other dependencies, and there are some kinks to work out though.

Drops grunt-recess and adds grunt-contrib-less.
Adds CSScomb for property ordering (non functional for some reason right now—halp?).
Requires addition of grunt-banner though since grunt-contrib-less hasn't implemented (and won't) banner support.

We'll need to integrate #11778 as part of this as well, either before or after. Perhaps @XhmikosR wants to join in on the fun here? Definitely could use some help.

mdo · 2013-12-09T03:14:53Z

I've asked the grunt-csscomb folks about the options here: csscomb/grunt-csscomb#9. Still can't figure that part out.

mdo · 2013-12-09T03:20:56Z

Also, meant to point to #11779 as the one to integrate with. At any rate, this has moving pieces 😀.

cvrebert · 2013-12-09T04:56:07Z

@mdo I pushed a commit with the fixes suggested by the grunt-csscomb folks; seems to work!

cvrebert · 2013-12-09T04:57:00Z

Gruntfile.js

-        dest: 'dist/css/<%= pkg.name %>-theme.min.css'
+        files: {
+          'dist/css/<%= pkg.name %>.sorted.css': ['dist/css/<%= pkg.name %>.css'],
+          'dist/css/<%= pkg.name %>.min.sorted.css': ['dist/css/<%= pkg.name %>.min.css'],


Why bother combing minified files?

Might as well be consistent :).

I screwed up—I get it now. Removed awhile back <3.

Conflicts: dist/css/bootstrap.min.css

Original discussion: less/less.js#1437 (comment). Since we’re switching to `grunt-contrib-less`, we can take advantage of newer LESS features than what RECESS supported. Included in that is the ability to `:extend`, and not only that, but `:extend(.mixin-name all)`. By doing so, we remove duplicate CSS for all our elements that were being clearfix-ed. Fixes #8947, #8968, #8991, #9257, #9268, #9291, #9430, #9604, #9686, #9929, #10731, #10793, #11305, #11498, #11533, #11570, #11604, #11652. (dem issues, tho)

mdo · 2013-12-09T07:19:05Z

This is looking super good at this point—not much difference between the Recess version in master and this one. Definitely can make this happen with v3.1.0.

Also just pushed a fix for #8947, and like a dozen other issues, for the duplicate CSS problem. Aww yeah.

Conflicts: Gruntfile.js dist/css/bootstrap-theme.min.css dist/css/bootstrap.min.css

Conflicts: Gruntfile.js package.json

mdo · 2013-12-09T07:57:23Z

Just merged in #11778 and #11779 to consolidate all the Grunt changes.

@cvrebert @XhmikosR Any improvements or tweaks you'd make at this point?

XhmikosR · 2013-12-09T08:06:01Z

Gruntfile.js

        options: {
-          compress: true
+          keepSpecialComments: 1,


Do you really want normalize.css' license header in the minified files? Also, now the banner is missing from the minified files.

https://github.com/XhmikosR/bootstrap/commit/fdc5d1dc34bf28e55c6359aeb098e608585ca6cb#diff-35b4a816e0441e6a375cd925af50752cR88

I don't see why not honestly. It's their code after all—credit due and all that. I'm open to feedback on this though. Why remove it, and is it even possible to do with grunt-contrib-less?

It's your call if you want the license info in the minified files, I guess the correct thing would be to keep it.

About the options, ieCompat seems something that should be enabled in grunt-contrib-less. Can't find the docs about setting clean-css' options via grunt-contrib-less atm.

cvrebert · 2013-12-09T08:06:48Z

@mdo I don't think we should have merged #11778. We should just enable grunt-contrib-less's clean-css-based minification options instead: https://github.com/gruntjs/grunt-contrib-less#cleancss

XhmikosR · 2013-12-09T08:10:18Z

I agree with the above @cvrebert. That's what I meant it seemed better to switch to grunt-contrib-less, because it combines both things.

mdo · 2013-12-09T08:16:13Z

Ah, good call. A sign it's too late to be doing this :D. Pulled it and added the cleancss option. Thanks fellas <3.

Let me know if there's anything else.

cvrebert · 2013-12-09T08:23:01Z

Enable source maps?
~~report: 'min' ?~~
strictMath: true ?

XhmikosR · 2013-12-09T08:29:56Z

Yeah was looking at those too. About source maps, I wonder how that will work since clean-css doesn't support them.

mdo · 2013-12-09T08:37:43Z

Added strictMath: true, but Grunt crashed and burned apparently:

➜ ~/Sites/bootstrap (drop_recess_for_less) grunt dist
Running "clean:dist" (clean) task
Cleaning dist...OK

Running "less:compile" (less) task
FATAL ERROR: JS Allocation failed - process out of memory

I've already added report: 'min' as well—it's under the minify subtask. Should be the only place we need to show it, right?

I'm fine calling this good enough, too, and we can explore those other things afterwards. I'm not familiar enough with source maps (yet). This is already a pretty huge win :).

Two final questions:

Does it make sense to put csslint that high up in the document? Seems like it should come after the compilation.
It sucks we have to use usebanner, but I saw no other option for consistent banner adding. Is this legit?

<3

cvrebert · 2013-12-09T08:43:27Z

(1) Agreed, it should be moved further down.

I've already added report: 'min' as well

Derp, my bad.

XhmikosR · 2013-12-09T08:50:26Z

@mdo: how about the ieCompat option?

About csslint, since we are running it against the compiled dist css I guess you could move it. I added it there after jshint and jscs since it's for a similar job.

cvrebert · 2013-12-09T08:54:11Z

@XhmikosR ieCompat defaults to true anyway

XhmikosR · 2013-12-09T08:56:11Z

Oh ok didn't notice that.

tlindig · 2013-12-09T14:26:26Z

Requires addition of grunt-banner though since grunt-contrib-less hasn't implemented (and won't) banner support

You could do it in same way you do it for js files, use concat task. No need for grunt-banner.

mdo · 2013-12-09T19:30:09Z

@tlindig Not a bad idea, I'll give that a go in a separate effort unless someone else wants to jump in :).

Drop grunt-recess for grunt-contrib-less

See twbs/bootstrap#11790

Drop grunt-recess for grunt-contrib-less

XhmikosR and others added 4 commits December 8, 2013 13:05

Minify css with clean-css.

fdc5d1d

Add csslint support.

cbfbf1b

Drop grunt-recess for grunt-contrib-less & co

c020042

restore original options value

f1cb57c

adding json

6b55e93

fixups to csscomb grunt task

dbe95f0

cvrebert reviewed Dec 9, 2013
View reviewed changes

mdo added 7 commits December 8, 2013 22:11

merge, compile

aae9c2d

derp, don't sort min /cc @cvrebert

4540f9e

single array == no rogue spaces

e90ce1c

specify block indent as numeral to prevent fubared media query output

0991c7c

more closely align property order with recess

e4c1ef7

Merge branch 'master' into drop_recess_for_less

2a4621b

Conflicts: dist/css/bootstrap.min.css

Merge branch 'pr/11778' into drop_recess_for_less

ea61a19

Conflicts: Gruntfile.js dist/css/bootstrap-theme.min.css dist/css/bootstrap.min.css

mdo mentioned this pull request Dec 9, 2013

Minify css with clean-css #11778

Closed

Merge branch 'pr/11779' into drop_recess_for_less

de218bb

Conflicts: Gruntfile.js package.json

mdo mentioned this pull request Dec 9, 2013

Add csslint support #11779

Closed

XhmikosR reviewed Dec 9, 2013
View reviewed changes

grunt

b9b17bc

Undo merge of #11778 and use cleancss min option

d466d52

mdo added a commit that referenced this pull request Dec 9, 2013

Merge pull request #11790 from twbs/drop_recess_for_less

25ec09f

Drop grunt-recess for grunt-contrib-less

mdo merged commit 25ec09f into master Dec 9, 2013

mdo deleted the drop_recess_for_less branch December 9, 2013 19:30

mdo mentioned this pull request Dec 9, 2013

v3.1.0 ship list #11734

Closed

cvrebert mentioned this pull request Dec 9, 2013

less.js with strictMath enabled runs out of memory trying to compile Bootstrap less/less.js#1725

Closed

cvrebert mentioned this pull request Feb 7, 2014

.row and .container class before and after elements print twice #12623

Closed

interactivellama added a commit to ExactTarget/fuelux that referenced this pull request Apr 3, 2014

BS3 switched to grunt-contib-less and away from recess

fed659b

See twbs/bootstrap#11790

interactivellama mentioned this pull request Apr 3, 2014

BS3 switched to grunt-contib-less and away from recess ExactTarget/fuelux#444

Merged

stempler pushed a commit to stempler/bootstrap that referenced this pull request Apr 11, 2014

(less) Merge pull request twbs#11790 from twbs/drop_recess_for_less

5db9c76

Drop grunt-recess for grunt-contrib-less

stempler pushed a commit to stempler/bootstrap that referenced this pull request Nov 4, 2014

(less) Merge pull request twbs#11790 from twbs/drop_recess_for_less

da9c60c

Drop grunt-recess for grunt-contrib-less

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop grunt-recess for grunt-contrib-less #11790

Drop grunt-recess for grunt-contrib-less #11790

mdo commented Dec 9, 2013

mdo commented Dec 9, 2013

mdo commented Dec 9, 2013

cvrebert commented Dec 9, 2013

cvrebert Dec 9, 2013

mdo Dec 9, 2013

mdo Dec 9, 2013

mdo commented Dec 9, 2013

mdo commented Dec 9, 2013

XhmikosR Dec 9, 2013

mdo Dec 9, 2013

XhmikosR Dec 9, 2013

cvrebert commented Dec 9, 2013

XhmikosR commented Dec 9, 2013

mdo commented Dec 9, 2013

cvrebert commented Dec 9, 2013

XhmikosR commented Dec 9, 2013

mdo commented Dec 9, 2013

cvrebert commented Dec 9, 2013

XhmikosR commented Dec 9, 2013

cvrebert commented Dec 9, 2013

XhmikosR commented Dec 9, 2013

tlindig commented Dec 9, 2013

mdo commented Dec 9, 2013

Drop grunt-recess for grunt-contrib-less #11790

Drop grunt-recess for grunt-contrib-less #11790

Conversation

mdo commented Dec 9, 2013

mdo commented Dec 9, 2013

mdo commented Dec 9, 2013

cvrebert commented Dec 9, 2013

cvrebert Dec 9, 2013

Choose a reason for hiding this comment

mdo Dec 9, 2013

Choose a reason for hiding this comment

mdo Dec 9, 2013

Choose a reason for hiding this comment

mdo commented Dec 9, 2013

mdo commented Dec 9, 2013

XhmikosR Dec 9, 2013

Choose a reason for hiding this comment

mdo Dec 9, 2013

Choose a reason for hiding this comment

XhmikosR Dec 9, 2013

Choose a reason for hiding this comment

cvrebert commented Dec 9, 2013

XhmikosR commented Dec 9, 2013

mdo commented Dec 9, 2013

cvrebert commented Dec 9, 2013

XhmikosR commented Dec 9, 2013

mdo commented Dec 9, 2013

cvrebert commented Dec 9, 2013

XhmikosR commented Dec 9, 2013

cvrebert commented Dec 9, 2013

XhmikosR commented Dec 9, 2013

tlindig commented Dec 9, 2013

mdo commented Dec 9, 2013