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

docs-assets: concatenate and minify all docs files. #11977

Merged
merged 6 commits into from
Jan 11, 2014
Merged

docs-assets: concatenate and minify all docs files. #11977

merged 6 commits into from
Jan 11, 2014

Conversation

XhmikosR
Copy link
Member

See #11976, docs.js has the normal BS banner atm.

@cvrebert
Copy link
Collaborator

IMHO, we may want to leave bootstrap.min.js separate, like we do with the CSS.

'docs-assets/js/holder.js',
'docs-assets/js/application.js'
],
dest: 'docs-assets/js/docs.js'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably be docs.min.js

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK thought about it too but left it since customize.js doesn't include "min" either.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, it also ought to be customize.min.js; one of those little things that no one has gotten around to.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I will change those now. I solved the issue with the CSS so I will add this too.

@XhmikosR
Copy link
Member Author

I plan to combine the CSS too but I haven't finished it yet.

EDIT: I can't make it work right with clean-css' root option. If I use root: '../dist/' I end up with url(C:/Users/xmr/Desktop/bootstrap/dist/fonts/glyphicons-halflings-regular.eot)

@XhmikosR
Copy link
Member Author

@cvrebert: please check now, I updated the PR.

'docs-assets/css/pygments-manni.css'
],
dest: 'docs-assets/css/pack.min.css'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra line

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@mdo
Copy link
Member

mdo commented Dec 23, 2013

I'd prefer to leave the compiled Bootstrap files separated from any built files. Helps ensure consistency across all our docs, examples, etc. We can combine the docs and Pygments CSS though into one file, perhaps manually even.

I don't know how much this really saves us—we have no hosting costs and the loading times aren't that bad right now with things as they are. Mostly what I see is more Grunt tasks.

@XhmikosR
Copy link
Member Author

@mdo: I don't mind leaving bootstrap.css out and concatenate the rest. But we should definitely do this. It's not only about bandwidth from your side; it's about bandwidth from the user's side.

@mdo
Copy link
Member

mdo commented Dec 23, 2013

Word, I hear you on the minification at least. I'm curious what the changes in size are though—how much are we gaining (er, losing)?

@XhmikosR
Copy link
Member Author

For the CSS it's around 11KB and -2 HTTP requests.

For the JS it should be bigger but don't have any numbers yet.

@XhmikosR
Copy link
Member Author

@mdo: don't merge this yet. Yet another IE 8 bug with the CSS file size limit. I suppose on the server we won't see the issue since gzip is enabled but I'm currently looking into this.

An alternative way will be to keep 2 CSS files, bootstrap.min.css and pack.min.css. And for consistency we can do this for the JS too.

What do you think?

@XhmikosR
Copy link
Member Author

Turns out it's a bug in clean-css or grunt-contrib-cssmin. I need to investigate this more but when I use noAdvanced: true in cssmin options then it works fine otherwise the resulting CSS isn't parsed right by IE8.

So we still can combine everything when this issue is sorted.

@XhmikosR
Copy link
Member Author

Worked around the issue for the time being by disabling advanced optimizations. Everything seems to work fine here. I also kept bootstrap.min.css and bootstrap.min.js.

BTW, raw-files.js seems redundant now since it's being combined to customize.min.js.

@mdo
Copy link
Member

mdo commented Jan 1, 2014

Looking at this again, some more things came to mind.

  • Packaging the docs CSS means running a grunt command just to load new CSS changes.
  • How does this handle updates to the raw-files.js? That task comes later in the Gruntfile—at first glance, we'd be concatenating and minifying the old version.

I'm all about optimizing, but not if it makes things more cumbersome to work with.

@XhmikosR
Copy link
Member Author

XhmikosR commented Jan 1, 2014

Both can be resolved easily though.

I can have a look tomorrow just give me a couple of hints how you want this addressed.

@XhmikosR
Copy link
Member Author

XhmikosR commented Jan 2, 2014

PR rebased.

@mdo: what exactly you don't like at the moment? You already have to run grunt dist.

The only thing I find pointless is the raw-files.js. But I'd rather leave this to someone else.

@mdo mdo merged commit d06d61d into twbs:master Jan 11, 2014
@mdo mdo mentioned this pull request Jan 11, 2014
@XhmikosR XhmikosR deleted the minify-assets branch January 12, 2014 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants