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

Collapse multiple leading and trailing whitespaces into a single whitespace / Does not collapse multiple   #16

Conversation

siva-sundar
Copy link
Contributor

@siva-sundar siva-sundar commented Oct 17, 2017

This PR collapses multiple leading and trailing whitespaces into a single whitespace as mentioned below.

Actual content:

<div>
    <span>    a     </span>    
    <span>    b    c     </span>
</div>

Minified content:

<div><span> a </span> <span> b c </span></div>

…itespace

- Does not collapse multiple &nbsp; into a single whitespace
Copy link
Collaborator

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

whooops, forgot to press the "Submit Review" button 🤦‍♂️

sorry for the long silence!!

utils/helpers.js Outdated
};

const hasLeadingOrTrailingWhiteSpace = function(chars) {
chars = encodeURIComponent(chars);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are you encoding the characters here?

utils/helpers.js Outdated
module.exports = {
stripWhiteSpace,
isWhitespaceTextNode,
WHITESPACE,
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we export this? looks unused 🤔

utils/helpers.js Outdated
chars = encodeURIComponent(chars || '');
chars = chars.replace(matchHorizontalTABAndNewLineBegin, ' ')
.replace(matchHorizontalTABAndNewLineEnd, ' ');
return decodeURIComponent(chars);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've built a similar thing lately for qunit-dom mostly according to the HTML spec. Might make sense to reuse the same implementation here too. (see https://github.com/simplabs/qunit-dom/blob/master/lib/helpers/collapse-whitespace.js)

@siva-sundar
Copy link
Contributor Author

I have made the changes suggested. Please have a look at it.

utils/helpers.js Outdated
return chars.replace(leadingWhiteSpace, ' ').replace(trailingWhiteSpace, ' ');
};

const stripNoMinifyBlocks = function(nodes) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why did you not use the normal function syntax here? (e.g. function stripNoMinifyBlocks(nodes) { ... } instead of the const stripNoMinifyBlocks = function(nodes) { ... })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No specific reason, since stripNoMinifyBlocks is never going to change. I have used const. Shall I use the normal syntax here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, I think that would be best, thanks

@siva-sundar siva-sundar force-pushed the patch_collapse_leading_and_trailing_whitespace branch from bf775d7 to 5989383 Compare October 27, 2017 14:03
@siva-sundar
Copy link
Contributor Author

siva-sundar commented Oct 27, 2017

Please let me know if anything else needs to be changed.

@Turbo87 Turbo87 merged commit 7d2a563 into mainmatter:master Oct 27, 2017
@Turbo87
Copy link
Collaborator

Turbo87 commented Oct 27, 2017

thanks :)

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.

None yet

2 participants