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

feat: support setting currentColor directly #76

Closed
wants to merge 2 commits into from

Conversation

stipsan
Copy link

@stipsan stipsan commented Sep 11, 2020

Replacing currentColor instead of setting color as a root param minifies a bit better, especially on small SVGs.

@stipsan
Copy link
Author

stipsan commented Nov 12, 2020

@hudochenkov could you give this a look? I'm happy to resolve the merge conflicts. But I'm not gonna invest that time if it's gonna happen again when v7 comes around 😉

@hudochenkov
Copy link
Collaborator

Honestly, I don't see the need of another option. I think this could be done with existing features. E. g. with removeFill and removeStroke. Or with transform.

@stipsan
Copy link
Author

stipsan commented Nov 16, 2020

Hey @hudochenkov thanks for replying 😃

In our design system svg-load is used 129 times with currentColor in the svg (162 in total). You may not agree that this option is needed, but it allowed me to optimize out currentColor on all of them with a quick search/replace. And since I'm not using removeFill, removeStroke or transform our postcss conf and our stylesheets are free of any information on what svg is using currentColor where. Is it a <path fill="none" stroke="currentColor">, or a <polygon stroke="currentColor" fill="currentColor">? Or just a <path fill="currentColor">?
And what happens if the designers update any of them? Back to step 1, checking every changed SVG to ensure all is well.

That's why we went with the simplest approach and just added color to the root. Curious to how much we could reduce our generated CSS by replacing currentColor where used, instead of adding the color prop on the root tag, we ran a test.
The result is that the following search/replace:

    &--delete::before {
        background-image: svg-load(
            "24x24/delete.svg",
-            color: env(--color-text)
+            currentColor: env(--color-text)
        );
    }

With the resulting change in output:

.icon--delete:before {
-    background-image: url("data:image/svg+xml;charset=utf-8,%3Csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 24 24' color='%23767676'%3E%3Cpath fill='currentColor' fill-rule='evenodd' d='M6 19V7h12v12a2 2 0 01-2 2H8a2 2 0 01-2-2zM5 5a1 1 0 011-1h12a1 1 0 011 1v1H5V5zm5-2h4a1 1 0 011 1H9a1 1 0 011-1z'/%3E%3C/svg%3E");
+    background-image: url("data:image/svg+xml;charset=utf-8,%3Csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 24 24'%3E%3Cpath fill='%23767676' fill-rule='evenodd' d='M6 19V7h12v12a2 2 0 01-2 2H8a2 2 0 01-2-2zM5 5a1 1 0 011-1h12a1 1 0 011 1v1H5V5zm5-2h4a1 1 0 011 1H9a1 1 0 011-1z'/%3E%3C/svg%3E");
}

Reducing the generated CSS by 3kB

If all it takes me to search/replace color to currentColor then it's worth the work. But if you have to go line by line, icon by icon, with the existing options, I don't think anyone will do that. Unless forced by some supervisor that is punishing you in creative ways 😂

@hudochenkov
Copy link
Collaborator

Thank you for an extended answer!

Have you tried to use transform option? To be honest, I've never used it myself (I'm a maintainer, but I didn't create this plugin :)). I have a feeling, that it is possible to use it for your use case.

Maybe @TrySound has better idea.

@stipsan
Copy link
Author

stipsan commented Nov 17, 2020

Ah I must have missed it when reading the docs. It looks like it can almost do the job, it just needs to change its signature:

-options.transform(svg, path)
+options.transform(svg, path, params)

Then transform should in theory be able to solve the same problem 🤔 Gimmie a minute!

@stipsan
Copy link
Author

stipsan commented Nov 17, 2020

Turns out svg is a string, not the parsed XML DOM that the processors have access to 😞

@stipsan
Copy link
Author

stipsan commented Nov 23, 2020

Gonna continue this in a fork

@stipsan stipsan closed this Nov 23, 2020
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.

None yet

2 participants