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

[icons] Remove fill attribute from some icons #12111

Merged

Conversation

ChristiaanScheermeijer
Copy link
Contributor

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.

@oliviertassinari oliviertassinari self-assigned this Jul 15, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 17, 2018

Let's catch them all!

juil -17-2018 23-19-18

@ChristiaanScheermeijer
Copy link
Contributor Author

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.

@ChristiaanScheermeijer
Copy link
Contributor Author

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?

@oliviertassinari
Copy link
Member

@ChristiaanScheermeijer I have noticed some clip path issues as well. Feel free to push the fix in this pull request.

@oliviertassinari oliviertassinari force-pushed the fix/remove-fill-attributes branch 3 times, most recently from c214967 to 43d8496 Compare July 18, 2018 20:38
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 18, 2018

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.
I think that we are good to move to stable v2.0.0 for the icon package 🎉.

It's not perfect yet, I had to keep the empty paths because of some icons relying on a complex clip-path logic.

@oliviertassinari oliviertassinari force-pushed the fix/remove-fill-attributes branch from 43d8496 to 27f546f Compare July 18, 2018 20:45
@oliviertassinari oliviertassinari merged commit 77a2d77 into mui:master Jul 18, 2018
@oliviertassinari
Copy link
Member

@ChristiaanScheermeijer Thank you

@ChristiaanScheermeijer
Copy link
Contributor Author

Thanks @oliviertassinari. I had the following change which does removes all unneeded code:

  const input = data
    .replace(/ clip-path="url\(#[abcd]\)"/g, '')
    .replace(/ fill-rule="evenodd"/g, '')
    .replace(/ clip-rule="evenodd"/g, '')
    .replace(/ fill="#010101"/g, '');

produces (e.g. AddAPhoto.js):

<React.Fragment><path d="M3 4V1h2v3h3v2H5v3H3V6H0V4h3zm3 6V7h3V4h7l1.83 2H21c1.1 0 2 .9 2 2v12c0 1.1-.9 2-2 2H5c-1.1 0-2-.9-2-2V10h3zm7 9c2.76 0 5-2.24 5-5s-2.24-5-5-5-5 2.24-5 5 2.24 5 5 5zm-3.2-5c0 1.77 1.43 3.2 3.2 3.2s3.2-1.43 3.2-3.2-1.43-3.2-3.2-3.2-3.2 1.43-3.2 3.2z" /></React.Fragment>

@ChristiaanScheermeijer ChristiaanScheermeijer deleted the fix/remove-fill-attributes branch July 19, 2018 07:38
@oliviertassinari
Copy link
Member

@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.

@ChristiaanScheermeijer
Copy link
Contributor Author

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:

Diff:
cloudcircletwotone_diff

Original:
cloudcircletwotone_original

Optimized (image):
cloudcircletwotone_target

Optimized (svg):

<?xml version="1.0" encoding="utf-8"?><svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"><path fill="none" d="M0 0h24v24H0V0z" /><path d="M12 4c-4.41 0-8 3.59-8 8s3.59 8 8 8 8-3.59 8-8-3.59-8-8-8zm4.08 12H8.5a3.5 3.5 0 1 1-.38-6.98 4.373 4.373 0 0 1 8.17 1.16c1.52.1 2.71 1.35 2.71 2.89 0 1.62-1.31 2.93-2.92 2.93z" opacity=".3" /><path d="M12 2C6.48 2 2 6.48 2 12s4.48 10 10 10 10-4.48 10-10S17.52 2 12 2zm0 18c-4.41 0-8-3.59-8-8s3.59-8 8-8 8 3.59 8 8-3.59 8-8 8z" /><path d="M16.29 10.19a4.373 4.373 0 1 0-8.17-1.16A3.483 3.483 0 0 0 5 12.5C5 14.43 6.57 16 8.5 16h7.58c1.61 0 2.92-1.31 2.92-2.92 0-1.54-1.2-2.79-2.71-2.89zM16 14H8.5c-.83 0-1.5-.67-1.5-1.5S7.67 11 8.5 11h.9l.49-1.05a2.377 2.377 0 0 1 4.44.63l.28 1.42H16c.55 0 1 .45 1 1s-.45 1-1 1z" /></svg>

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 21, 2018

@ChristiaanScheermeijer Thanks for spotting this issue! It's a SVGO issue. It's trivial to fix, I'm on it.

capture d ecran 2018-07-21 a 15 51 18

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: icons Specific to @mui/icons
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants