-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[icons] Remove fill attribute from some icons #12111
[icons] Remove fill attribute from some icons #12111
Conversation
All hardcoded fills should be removed now. I also moved the regex replacement before the svgo optimize call so that empty groups (after fill removal) are deleted as well. |
I've already made a fix for 52 icons which are broken due to missing clipPaths references. Do you want me to push this fix to this PR as well? |
@ChristiaanScheermeijer I have noticed some clip path issues as well. Feel free to push the fix in this pull request. |
c214967
to
43d8496
Compare
Alright, I had to change the regexp logic so we have all the icons displaying correctly. Some icons were broken. I have also fixed the react warnings. I can confirm that the color issue is fixed. It's not perfect yet, I had to keep the empty paths because of some icons relying on a complex clip-path logic. |
43d8496
to
27f546f
Compare
@ChristiaanScheermeijer Thank you |
Thanks @oliviertassinari. I had the following change which does removes all unneeded code:
produces (e.g. AddAPhoto.js):
|
@ChristiaanScheermeijer I have chosen the safest path so far, removing as little codes as possible so the SVG rendering stays correct. I haven't tried what you are suggesting, maybe this would work, be we need to verify the 5k icons still display as expected. |
Fair enough, I did some experiments comparing the downloaded icons against the generated icons. The only issues I've found have something to do with the svgo convertPathData plugin. It messes up 28 icons. CloudCircleTwoTone for example: Optimized (svg):
|
@ChristiaanScheermeijer Thanks for spotting this issue! It's a SVGO issue. It's trivial to fix, I'm on it. |
This PR removes the fill attribute in some icons which disallows setting the icon color using CSS/JSS.
note While testing I'd noticed that the NextWeek icon is broken. Since this is a different issue
(also in latest release), I will try to fix this in a different PR.