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.

Sign up for GitHub

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

Jump to bottom

[Icons] Misc fixes and optimizations #12036

Merged
merged 2 commits into from
Jul 3, 2018
Merged

[Icons] Misc fixes and optimizations #12036

merged 2 commits into from
Jul 3, 2018

Conversation

mbrookes
Copy link
Member

@mbrookes mbrookes commented Jul 2, 2018 •
edited
Loading

  • No longer strip <g> & </g>
  • Use react fragment rather than <g></g> in the template.
  • Fix a bug in the rename filter that broke the build.
  • Drop legacy ic_ from the svg filenames.
  • Strip empty <defs</defs>

@mbrookes mbrookes requested a review from oliviertassinari July 2, 2018 22:48
kybarg
kybarg reviewed Jul 2, 2018
View reviewed changes
packages/material-ui-icons/src/AirplayTwoTone.js Outdated
@@ -2,5 +2,5 @@ import React from 'react';
import createSvgIcon from './utils/createSvgIcon';

export default createSvgIcon(
<g><g><path d="M6 22h12l-6-6z" /><path d="M21 3H3c-1.1 0-2 .9-2 2v12c0 1.1.9 2 2 2h4v-2H3V5h18v12h-4v2h4c1.1 0 2-.9 2-2V5c0-1.1-.9-2-2-2z" /></g>
<><g fill="none"></g><path d="M6 22h12l-6-6z" /><path d="M21 3H3c-1.1 0-2 .9-2 2v12c0 1.1.9 2 2 2h4v-2H3V5h18v12h-4v2h4c1.1 0 2-.9 2-2V5c0-1.1-.9-2-2-2z" /></>
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks wrong 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

It does, but it isn't: https://reactjs.org/docs/fragments.html

Copy link
Member

Choose a reason for hiding this comment

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

I would vote for using React.Fragment, it's more explicit.

Copy link
Contributor

@kybarg kybarg Jul 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@mbrookes thank you for pointing that out 🙂

packages/material-ui-icons/src/AlarmAddSharp.js Outdated
@@ -2,5 +2,5 @@ import React from 'react';
import createSvgIcon from './utils/createSvgIcon';

export default createSvgIcon(
<g><path d="M17.337 1.811l4.607 3.844-1.281 1.535-4.608-3.843zM6.663 1.81l1.282 1.536L3.337 7.19 2.056 5.654zM12 4a9 9 0 1 0 .001 18.001A9 9 0 0 0 12 4zm0 16c-3.86 0-7-3.14-7-7s3.14-7 7-7 7 3.14 7 7-3.14 7-7 7z" /><path d="M13 9h-2v3H8v2h3v3h2v-3h3v-2h-3z" /></g>
<><g><path d="M17.337 1.811l4.607 3.844-1.281 1.535-4.608-3.843zM6.663 1.81l1.282 1.536L3.337 7.19 2.056 5.654zM12 4a9 9 0 1 0 .001 18.001A9 9 0 0 0 12 4zm0 16c-3.86 0-7-3.14-7-7s3.14-7 7-7 7 3.14 7 7-3.14 7-7 7z" /><path d="M13 9h-2v3H8v2h3v3h2v-3h3v-2h-3z" /></g></>
Copy link
Contributor

Choose a reason for hiding this comment

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

And in other files

@oliviertassinari oliviertassinari merged commit b3564a7 into mui:master Jul 3, 2018
@oliviertassinari
Copy link
Member

Published https://www.npmjs.com/package/@material-ui/icons/v/2.0.0-beta.1

@mbrookes mbrookes deleted the update-icons-2 branch July 3, 2018 09:54
@ranjithprabhuk
Copy link

@mbrookes , @oliviertassinari, @kybarg
I verified the published release, i can see that few icons were missing for the default filled (icons like settings are available for Outlined, TwoTone, etc., but missing for Filled).
Also I hope that the icon should have a variant prop and internally in material-ui icons we can handle the icon based on the props.
Eg:
By default variant we can set it to filled.

I hope this will reduce the import pain of multiple icons and we also don't want to maintain a huge list of files for all the icons (currently for one icon we have 5 files).

Please correct if I am wrong.

@mbrookes
Copy link
Member Author

mbrookes commented Jul 4, 2018

@ranjithprabhuk Since users would typically only be using one theme, putting all 5 themes in one icon would both bloat the components while failing to reduce the number of imports.

acroyear pushed a commit to acroyear/material-ui that referenced this pull request Jul 14, 2018
@mbrookes
[Icons] Misc fixes and optimizations (mui#12036)
d3ab7ab 
* [Icons] Misc fixes and optimizations

* Use explicit React.Fragment so as not to cunfuse newbies 😜
@oliviertassinari oliviertassinari mentioned this pull request Jul 24, 2018
Closed
2 tasks
@snyk-bot snyk-bot mentioned this pull request Sep 30, 2019
Open
This was referenced Mar 26, 2020
Merged
Open
@zannager zannager added the package: icons Specific to @mui/icons label Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kybarg kybarg kybarg left review comments

@oliviertassinari oliviertassinari oliviertassinari approved these changes

Assignees
No one assigned
Labels
package: icons Specific to @mui/icons
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mbrookes @oliviertassinari @ranjithprabhuk @kybarg @zannager