#722: introduce collapseCustomFragments option
#723
Merged
+75
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ace around `ignoreCustomFragments` (default: false).
alexlamsl
requested changes
Sep 24, 2016
Collaborator
alexlamsl
left a comment
alexlamsl
left a comment
There was a problem hiding this comment.
A couple of suggestions, otherwise LGTM.
README.md
Outdated
| | `collapseBooleanAttributes` | [Omit attribute values from boolean attributes](http://perfectionkills.com/experimenting-with-html-minifier/#collapse_boolean_attributes) | `false` | | ||
| | `collapseInlineTagWhitespace` | Don't leave any spaces between `display:inline;` elements when collapsing. Must be used in conjunction with `collapseWhitespace=true` | `false` | | ||
| | `collapseWhitespace` | [Collapse white space that contributes to text nodes in a document tree](http://perfectionkills.com/experimenting-with-html-minifier/#collapse_whitespace) | `false` | | ||
| | `collapseCustomFragments` | Collapse white space around `ignoreCustomFragments`. | `false` | |
Collaborator
There was a problem hiding this comment.
Given that you are removing whitespaces surrounding the custom fragments indiscriminately, may I suggest calling this trimCustomFragments?
README.md
Outdated
| | `collapseBooleanAttributes` | [Omit attribute values from boolean attributes](http://perfectionkills.com/experimenting-with-html-minifier/#collapse_boolean_attributes) | `false` | | ||
| | `collapseInlineTagWhitespace` | Don't leave any spaces between `display:inline;` elements when collapsing. Must be used in conjunction with `collapseWhitespace=true` | `false` | | ||
| | `collapseWhitespace` | [Collapse white space that contributes to text nodes in a document tree](http://perfectionkills.com/experimenting-with-html-minifier/#collapse_whitespace) | `false` | | ||
| | `collapseCustomFragments` | Collapse white space around `ignoreCustomFragments`. | `false` | |
Collaborator
There was a problem hiding this comment.
Given that you are removing whitespaces surrounding the custom fragments indiscriminately, may I suggest calling this trimCustomFragments?
| assert.equal(minify(input, { | ||
| collapseWhitespace: true, | ||
| collapseCustomFragments: true | ||
| }), output); |
Collaborator
There was a problem hiding this comment.
May I get a few more tests here with:
- whitespaces before and/or after
<? ... ?> - contents before and/or after
<? ... ?> - consecutive custom fragments
Contributor
Author
|
Renamed |
alexlamsl
approved these changes
Sep 25, 2016
Collaborator
|
@adamchal thanks a lot! |
Contributor
Author
|
@alexlamsl no problem. Thanks for maintaining this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…in order to collapse white space around
ignoreCustomFragments(default: false). This resolves issue #722.Default behavior introduced from #436 is preserved. Only setting
collapseCustomFragmentstotruewill collapse the white space.Note: I did not add the the compiled files in
dist/in order to avoid merge conflict hell. Assumed you didn’t want that included in PRs.