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] Modernize icons package #7203

Merged
merged 2 commits into from Jun 23, 2017
Merged

[Icons] Modernize icons package #7203

merged 2 commits into from Jun 23, 2017

Conversation

kvet
Copy link
Contributor

@kvet kvet commented Jun 21, 2017

This PR fixes the following problems:

  • Reexporting icons from the index.js file of the material-ui-icons package did not work. Icons were reexported as named exports but defined as default export.
  • Tree-shaking did not work. The package did not expose ES2015 module entry point.

"build:babel": "babel --presets es2015,react -d ./build ./src",
"build:babel:es2015module": "BABEL_ENV=modules babel ./src/index.js --presets=../../../scripts/material-ui-babel-preset-es2015 --out-file ./build/index.es.js",
Copy link
Member

Choose a reason for hiding this comment

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

Some naive question as I haven't tried.
Would copy pasting the build commands for the main package work?
https://github.com/callemall/material-ui/blob/next/package.json#L26-L27

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is possible. But it could be a complication. In the root .babelrc file there are many plugins that seem redundant. So, if we will choose this option, it will require to list all of the babel plugins in the 'material-ui-icons' package development dependencies.

As an alternative, it is possible to clean up changes in my PR to match the 'material-ui' package scripts.

Copy link
Member

Choose a reason for hiding this comment

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

Why would we need to list all of the babel plugins in the 'material-ui-icons' package development dependencies? People need to install the root dependencies beforehand.

I would rather have things as symetric as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I understand your point. I will prepare additional changes.

@oliviertassinari
Copy link
Member

@kvet Thank you

@kvet kvet deleted the modernize-icons-package branch June 23, 2017 17:28
@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
Labels
package: icons Specific to @mui/icons
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants