-
Notifications
You must be signed in to change notification settings - Fork 572
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 "ol" and "ul" to nonPhrasing #889
Conversation
That's not how testing works. Please add tests which fails in the current version but passes with your changes in this PR. |
Please exclude any changes to |
tests/index.html
Outdated
@@ -3,12 +3,12 @@ | |||
<head> | |||
<meta charset="utf-8"> | |||
<title>HTML Minifier Tests</title> | |||
<link rel="stylesheet" href="https://code.jquery.com/qunit/qunit-2.5.0.css"> | |||
<link rel="stylesheet" href="https://code.jquery.com/qunit/qunit-2.5.1.css"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also exclude changes to this file.
|
||
input = '<p>a<ol><li>item</li></ol></p>'; | ||
output = '<p>a</p><ol><li>item</li></ol><p></p>'; | ||
assert.equal(minify(input, { html5: true }), output); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests look good, thanks.
Can you add one more that also does removeOptionalTags: true
to make sure the newly placed </p>
doesn't get removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The generated </p> before ul/ol should be removed because it's optional. I also added test to check the <p></p> at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect - thanks! 👍
Sorry for added dist by mistake. |
Closing issue #888.
I didn't change any tests in this PR because there's no current test covering which tag belongs to non-phrasing tags.