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.

Sign up for GitHub

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

Jump to bottom

Add grunt-contrib-htmlmin. #15294

Merged
merged 1 commit into from
Apr 8, 2015
Merged

Add grunt-contrib-htmlmin. #15294

merged 1 commit into from
Apr 8, 2015

Conversation

XhmikosR
Copy link
Member

@XhmikosR XhmikosR commented Dec 4, 2014

Only used for the GitHub docs.

Running "htmlmin:dist" (htmlmin) task
Minified _gh_pages/about/index.html 18.37 kB → 15.38 kB
Minified _gh_pages/browser-bugs/index.html 17.47 kB → 14.48 kB
Minified _gh_pages/components/index.html 298.71 kB → 258.74 kB
Minified _gh_pages/css/index.html 305.7 kB → 277.57 kB
Minified _gh_pages/customize/index.html 142.77 kB → 125.54 kB
Minified _gh_pages/getting-started/index.html 67.7 kB → 62.09 kB
Minified _gh_pages/index.html 11.84 kB → 9.97 kB
Minified _gh_pages/javascript/index.html 199.8 kB → 179.11 kB
Minified _gh_pages/migration/index.html 25.08 kB → 19.61 kB

I skipped the examples on purpose since many people seem to copy paste those. Let me know your thoughts.

@XhmikosR XhmikosR added this to the v3.3.2 milestone Dec 4, 2014
@XhmikosR
Copy link
Member Author

XhmikosR commented Dec 4, 2014

I need to check why validation fails later.

@cvrebert
Copy link
Collaborator

cvrebert commented Dec 4, 2014

-1 from me. Don't think it's worth risking the bugs that this might cause.

@XhmikosR
Copy link
Member Author

XhmikosR commented Dec 4, 2014

Well, there shouldn't be any issues, because 1) I have enabled conservative whitespace removal and 2) the validator should catch any issues.

The gain isn't small IMO; I'm not talking about the index page here ofc.

@mdo
Copy link
Member

mdo commented Dec 4, 2014

There are differences that can come up from HTML formatting and white space—in particular with inline-block if memory serves. I'd be interested in seeing how this looks though as I'd want to be convinced the bandwidth savings are worth any potential hiccups.

@XhmikosR
Copy link
Member Author

XhmikosR commented Dec 4, 2014

Like I said, I have enabled conservative whitespace; there shouldn't be any issue with inline-block.

Feel free to check out the branch; I'll have a look at the errors tomorrow or so.

cvrebert
cvrebert reviewed Dec 4, 2014
View reviewed changes
Gruntfile.js
@@ -391,7 +411,7 @@ module.exports = function (grunt) {
require('time-grunt')(grunt);

// Docs HTML validation task
grunt.registerTask('validate-html', ['jekyll:docs', 'validation']);
grunt.registerTask('validate-html', ['jekyll:docs', 'htmlmin', 'validation']);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure about this; this means it will report much less helpful line numbers for the locations of errors.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that's a very valid concern. I've run into these validation reports a lot as I write new docs sections.

@cvrebert
Copy link
Collaborator

cvrebert commented Dec 4, 2014

One of the validation errors is:

<p> <div class=highlight>(...stuff...)</div> <h3 id=js-noconflict>No conflict</h3> </p>

Pretty sure the <h3> implicitly closes the paragraph, which would make the </p> extraneous.
Interestingly, there's no <p> around the <div class="highlight"> in the original non-minified HTML.

@cvrebert
Copy link
Collaborator

@XhmikosR Rebase?

@XhmikosR
Copy link
Member Author

Sure thing. I'm just having some second thoughts regarding doing the
validation after the minification... And doing it twice will be a lot
slower. I'll think about it more...
On Dec 30, 2014 9:52 PM, "Chris Rebert" [email protected] wrote:

@XhmikosR https://github.com/XhmikosR Rebase?

—
Reply to this email directly or view it on GitHub
#15294 (comment).

@XhmikosR XhmikosR force-pushed the grunt-contrib-htmlmin branch 3 times, most recently from 569757f to b4019d7 Compare January 6, 2015 22:33
@mdo mdo modified the milestones: v3.3.2, v3.3.3 Jan 19, 2015
@XhmikosR XhmikosR force-pushed the grunt-contrib-htmlmin branch 5 times, most recently from 88e8382 to 109160e Compare January 26, 2015 16:11
@XhmikosR
Copy link
Member Author

@mdo @cvrebert: apparently, those validation errors we got in this branch were correct. I don't get how the validator didn't catch them in master...

So, I think this branch is completely safe. I removed the task from the validation target.

@XhmikosR XhmikosR force-pushed the grunt-contrib-htmlmin branch 2 times, most recently from e6135ce to c44d99b Compare February 2, 2015 20:24
@XhmikosR XhmikosR force-pushed the grunt-contrib-htmlmin branch 2 times, most recently from 5f910bf to ceabed0 Compare February 6, 2015 21:56
@XhmikosR XhmikosR force-pushed the grunt-contrib-htmlmin branch 5 times, most recently from 7c7ab37 to d15ccc9 Compare February 17, 2015 05:41
@mdo
Copy link
Member

mdo commented Feb 25, 2015

What's the deal on white-space sensitive components then? How does inline-block react with this kind of stuff?

@hnrch02
Copy link
Collaborator

hnrch02 commented Feb 25, 2015

@mdo #15294 (comment). conservativeCollapse always collapses whitespace to 1 space (and never removes it entirely). See https://github.com/kangax/html-minifier#options-quick-reference

@XhmikosR
Copy link
Member Author

Yeah, it keeps the whitespace so it's pretty much safe.

@kkirsche
Copy link
Contributor

@cvrebert you are correct about paragraphs. W3C Validator returns:

No p element in scope but a p end tag seen.

for

<p> <div class=highlight>(...stuff...)</div> <h3 id=js-noconflict>No conflict</h3> </p>

@XhmikosR XhmikosR removed this from the v3.3.4 milestone Mar 2, 2015
@mdo
Copy link
Member

mdo commented Mar 27, 2015

Sorry I missed that the white-space stuff was all settled. Is there another bug in this to address still?

@XhmikosR
Copy link
Member Author

@mdo: it should be OK as far as I'm concerned.

@mdo
Copy link
Member

mdo commented Mar 27, 2015

Cool, I'm okay to see this ship then if the white-space problem has been completely addressed. <3

@XhmikosR XhmikosR added this to the v3.3.5 milestone Mar 27, 2015
@XhmikosR
Add grunt-contrib-htmlmin.
e778dc1 
Only used for the GitHub docs.
XhmikosR added a commit that referenced this pull request Apr 8, 2015
@XhmikosR
Merge pull request #15294 from twbs/grunt-contrib-htmlmin
b9b824c 
Add grunt-contrib-htmlmin.
@XhmikosR XhmikosR merged commit b9b824c into master Apr 8, 2015
@XhmikosR XhmikosR deleted the grunt-contrib-htmlmin branch April 8, 2015 06:37
@cvrebert cvrebert mentioned this pull request Apr 8, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
build docs
Projects
None yet
Milestone
v3.3.5
Development

Successfully merging this pull request may close these issues.
5 participants
@XhmikosR @cvrebert @mdo @hnrch02 @kkirsche