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

Add max line length option #203

Closed
HugoHeneault opened this Issue Jul 7, 2014 · 7 comments

Comments

Projects
None yet
3 participants
@HugoHeneault

HugoHeneault commented Jul 7, 2014

Hello,

I use html-minifier for emails, and some mail servers automatically add a new line after 1000 caracters. This behaviour breaks html structure.

For example :

``` `

g="10"


It could be great if we can add an option to limit the line size without breaking the html. 
Do you think this is possible ?

Thanks.
@duncanbeevers

This comment has been minimized.

Show comment
Hide comment
@duncanbeevers

duncanbeevers Jul 10, 2014

Collaborator

@in4matik I knocked out a super-basic version of this functionality here.

It's a bit kludgy though I have some ideas about how to improve it, but it should basically accomplish what you're after.

I tested it against the source of this page with a maxLineLength setting of 900, and collapseWhitespace true, and this was the result.

Collaborator

duncanbeevers commented Jul 10, 2014

@in4matik I knocked out a super-basic version of this functionality here.

It's a bit kludgy though I have some ideas about how to improve it, but it should basically accomplish what you're after.

I tested it against the source of this page with a maxLineLength setting of 900, and collapseWhitespace true, and this was the result.

@kangax

This comment has been minimized.

Show comment
Hide comment
@kangax

kangax Jul 10, 2014

Owner

@duncanbeevers Looks pretty good! I'm only concerned about performance implications. Could you run node ./benchmark.js locally, and compare average time (reported at the end)?

Owner

kangax commented Jul 10, 2014

@duncanbeevers Looks pretty good! I'm only concerned about performance implications. Could you run node ./benchmark.js locally, and compare average time (reported at the end)?

@HugoHeneault

This comment has been minimized.

Show comment
Hide comment
@HugoHeneault

HugoHeneault Jul 10, 2014

Wow. Seems great @duncanbeevers !

I haven't time to test it, but I will ! Thanks.

HugoHeneault commented Jul 10, 2014

Wow. Seems great @duncanbeevers !

I haven't time to test it, but I will ! Thanks.

@duncanbeevers

This comment has been minimized.

Show comment
Hide comment
@duncanbeevers

duncanbeevers Jul 10, 2014

Collaborator

@kangax
Benchmark output here run with gh-pages as-is, then with limit-line-length but not limiting lines, and finally with a maxLineLength of 500.

Speed looks pretty stable, variance seems insignificant.

Collaborator

duncanbeevers commented Jul 10, 2014

@kangax
Benchmark output here run with gh-pages as-is, then with limit-line-length but not limiting lines, and finally with a maxLineLength of 500.

Speed looks pretty stable, variance seems insignificant.

@kangax

This comment has been minimized.

Show comment
Hide comment
@kangax

kangax Jul 10, 2014

Owner

Ok, let's get it in. Can you send a PR? Thanks.

Owner

kangax commented Jul 10, 2014

Ok, let's get it in. Can you send a PR? Thanks.

@duncanbeevers

This comment has been minimized.

Show comment
Hide comment
@duncanbeevers

duncanbeevers Jul 10, 2014

Collaborator

Sure. I was thinking of poking at this some more to get better-compressed results.

For example, right now it maximally-compresses

<a class="hey">link link</a>

to this

<a
 class="hey">
link link
</a>

I think the best we can hope for is this:

<a
class
=
"hey"
>
link
link
</a
>

But that's a fish to fry another day!

Collaborator

duncanbeevers commented Jul 10, 2014

Sure. I was thinking of poking at this some more to get better-compressed results.

For example, right now it maximally-compresses

<a class="hey">link link</a>

to this

<a
 class="hey">
link link
</a>

I think the best we can hope for is this:

<a
class
=
"hey"
>
link
link
</a
>

But that's a fish to fry another day!

@duncanbeevers

This comment has been minimized.

Show comment
Hide comment
@duncanbeevers

duncanbeevers Jul 22, 2014

Collaborator

I think this issue can be closed. If we want to pursue more aggressive splitting, I suggest we address that with another more-focused PR.

Collaborator

duncanbeevers commented Jul 22, 2014

I think this issue can be closed. If we want to pursue more aggressive splitting, I suggest we address that with another more-focused PR.

@kangax kangax closed this Jul 22, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment