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

Simplify minify regular expression #141

Merged
merged 1 commit into from
Oct 15, 2016
Merged

Simplify minify regular expression #141

merged 1 commit into from
Oct 15, 2016

Conversation

pathawks
Copy link
Member

No description provided.

@benbalter
Copy link
Contributor

I see a | minify filter in our future.

@pathawks
Copy link
Member Author

I see a | minify filter in our future.

Not a bad idea, but the trick is that is kind of has to be a context aware minify. It is easy to minify these templates before they are rendered because we know that there is nowhere in the template where whitespace is significant. Once we start dealing with user generated content, we never know when we might run into a <pre> or similar where we cannot just blindly strip whitespace.

@pathawks
Copy link
Member Author

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 3af8f29 into master Oct 15, 2016
@jekyllbot jekyllbot deleted the pr/regex branch October 15, 2016 18:54
jekyllbot added a commit that referenced this pull request Oct 15, 2016
@pathawks pathawks removed the fix label Oct 15, 2016
# 1. A '>', which closes an XML tag or
# 2. A '}', which closes a Liquid tag
# We will strip all of this whitespace to minify the template
MINIFY_REGEX = %r!(?<=>|})\s+!
Copy link
Member Author

Choose a reason for hiding this comment

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

This could be changed to %r!(?<=[>}])\s+!

I don't know if that would be any better or worse 🐳

@jekyll jekyll locked and limited conversation to collaborators Apr 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants