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] New iteration #12170

Merged
merged 1 commit into from
Jul 18, 2018

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Jul 17, 2018

Speed up the google icons scrapping script and makes it more reliable.

@oliviertassinari oliviertassinari force-pushed the icons-new-iteration branch 3 times, most recently from b5b7a0a to 73464d3 Compare July 17, 2018 18:06
@oliviertassinari oliviertassinari force-pushed the icons-new-iteration branch 2 times, most recently from e923bba to bbf062f Compare July 17, 2018 18:29
@oliviertassinari oliviertassinari self-assigned this Jul 17, 2018
@oliviertassinari oliviertassinari force-pushed the icons-new-iteration branch 3 times, most recently from 6b9cf58 to a0fe99b Compare July 17, 2018 21:24
Copy link
Member

@mbrookes mbrookes left a comment

Choose a reason for hiding this comment

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

Not even going to attempt to review your code 😄, but it's working great, and looks like a great improvement! 🚀

@@ -57,6 +57,10 @@ There are three exceptions to this rule:

{{"demo": "pages/style/icons/SvgMaterialIcons.js"}}

The full list:
Copy link
Member

Choose a reason for hiding this comment

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

lol

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm reverting the demo. It's significantly slowing everything down. I'm gonna keep it commented so we can use it to debug.

@oliviertassinari oliviertassinari force-pushed the icons-new-iteration branch 2 times, most recently from 5128ecb to 4ba8871 Compare July 18, 2018 18:01
@oliviertassinari oliviertassinari merged commit c7cc6a8 into mui:master Jul 18, 2018
@oliviertassinari oliviertassinari deleted the icons-new-iteration branch July 18, 2018 18:13
@eddiemonge
Copy link
Contributor

Maybe this should have had a review. Why was babel runtime added as a dependency of Icons? c7cc6a8#diff-3166425e4a7cf7143755bf476e42250fR45

@oliviertassinari
Copy link
Member Author

@eddiemonge Because we use @babel/plugin-transform-runtime. Is that an issue?

@eps1lon
Copy link
Member

eps1lon commented Oct 13, 2018

@oliviertassinari See #13220

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Oct 13, 2018

Ugh. That is so dirty. The babel runtime transform should be adding that code itself instead of requiring another package.

@eddiemonge What's wrong in that?

A plugin that enables the re-use of Babel's injected helper code to save on codesize.

https://babeljs.io/docs/en/babel-plugin-transform-runtime

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.

5 participants