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

Remove <style> element from the minifyStyles plugin when it is empty #1373

Closed
wants to merge 1 commit into from

Conversation

yisibl
Copy link
Contributor

@yisibl yisibl commented Feb 24, 2021

Fixed: #1226

In the minifyStyles plugin, if the <style> is empty or the css rule in it is not applied to the current svg node, we should remove the empty <style> element.

A similar process has been done in the inlineStyles plugin.

if (style.cssAst.children.isEmpty()) {
// clean up now emtpy <style/>s
var styleParentEl = style.styleEl.parentNode;
styleParentEl.spliceContent(styleParentEl.content.indexOf(style.styleEl), 1);

Origin

<svg viewBox="0 0 100 100" xmlns="http://www.w3.org/2000/svg">
    <style></style>
    <style />
    <rect x="10" y="10" width="100" height="100"/>
</svg>

Before:

<svg viewBox="0 0 100 100" xmlns="http://www.w3.org/2000/svg">
    <style />
    <style />
    <rect x="10" y="10" width="100" height="100"/>
</svg>

After:

<svg viewBox="0 0 100 100" xmlns="http://www.w3.org/2000/svg">
    <rect x="10" y="10" width="100" height="100"/>
</svg>

@@ -6,7 +6,7 @@
<stop offset="1" stop-color="#6b5aed" />
</linearGradient>
</defs>
<path d="M46 24a21.9 21.9" fill="url(#file-name_svg__file-name_svg__original-id)"/>
<path d="M5 24a21.9 21.9 10 1 1 8 18" fill="url(#file-name_svg__file-name_svg__original-id)"/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

M46 24a21.9 21.9 is invalid path data, fixed it.

@@ -0,0 +1,22 @@
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 48 48">
<defs>
<style>/* test */</style>
Copy link
Contributor Author

@yisibl yisibl Feb 24, 2021

Choose a reason for hiding this comment

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

At present, the test here fails, and it actually generates:

<style>
</style>

I don't know how to solve it yet.

@strarsis
Copy link
Contributor

strarsis commented Mar 19, 2021

My two cents here: Wouldn't it make sense to add a plugin that just removes empty <style> elements?
As multiple plugins should now implement the same logic, it should make sense to put this into a dedicated plugin.

Related: #592 (comment)

@TrySound
Copy link
Member

@strarsis I think #1381 perfectly fits into this task since it combines multiple <style> content it may collapse <style> into none.

@yisibl
Copy link
Contributor Author

yisibl commented Mar 23, 2021

@strarsis If there can be better done, I support it.

@TrySound
Copy link
Member

This is implemented as part of new mergeStyles plugin. Thanks anyway!

@TrySound TrySound closed this Mar 28, 2021
@yisibl
Copy link
Contributor Author

yisibl commented Mar 29, 2021

Continue to improve: #1457

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

empty style tag not removed
3 participants