[icon-builder] Update to generate standalone package #6104

mbrookes · 2017-02-10T00:54:06Z

PR has tests / docs demo, and is linted.
Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

Closes #6074.

npm run build now follows a similar pattern the the Material-UI build process:

generate the icons in the build directory
generates a package.json
copies the LICENSE file

The icon file names are PascalCase in in a single flattened directory

The generated icons use an absolute path for the import, and the package.json has a dependancy in material-ui@>1.0.0.

Finally, the dependencies are updated, and some missing ones added.

mbrookes · 2017-02-10T01:24:01Z

Once this is published, I'll update the docs site to use the packaged icons, and remove any icons from material-ui/src that aren't otherwise needed by Material-UI components. Any that are left can go in internal/icons.

The docs will also need to reference the fact that the icons are now separate, and describe the naming convention. Thinking about it, the build dir needs a README too for npmjs.com to display.

One thing we need to decide on is the package name. It's mui-icons for now to be short and snappy, but I"m open to suggestions.

oliviertassinari

The icon-builder is quite an old piece of code from material-ui.That remembers me the beginning 😬 .

oliviertassinari · 2017-02-10T22:55:50Z

packages/icon-builder/LICENSE

@@ -0,0 +1,21 @@
+The MIT License (MIT)


What about using the one at the root?

Isn't this a separate package technically? (And not like the files ever going to change). I can have the script reach outside it's walls for it though if you prefer.

oliviertassinari · 2017-02-10T22:58:04Z

packages/icon-builder/package.json

-    "prebuild": "rimraf js jsx",
-    "build": "node build.js --output-dir ../../src/svg-icons --svg-dir ./node_modules/material-design-icons --glob '/**/production/*_24px.svg' --mui-require relative --renameFilter ./filters/rename/material-design-icons.js",
-    "test": "echo \"Error: no test specified\" && exit 1"
+  "name": "material-ui-icon-builder",


What about renaming the folder: material-ui-svg-icons as well as the name here?
I don't think that we need to expose the builder tools. Shouldn't they be internal scripts that help us publishing it?

I don't think that we need to expose the builder tools. Shouldn't they be internal scripts that help us publishing it?

Sorry, I don't understand. That's exactly what they are.

You are right, but the branding & documentation make me think otherwise:

This tool generates Material-UI SvgIcon components for a set of svg icons.

I think I see what you're getting at now. (It seemed - especially given your later comment about needing a React peerDepenency in this package's package.json - that you thought the scripts were being published as part of the npm package).

However, from the way they were written, it does appear that the intention was that this would be a toolset for users to be able to generate other icon libraries - in fact, I came across this one in the wild while looking at possible names for the generated icon set.

That we've now repurposed it to create a standalone build of the Material Design icons that happens to be published from here is kind of incidental to that I think, but tell me if I'm wrong - I often am!

it does appear that the intention was that this would be a toolset for users to be able to generate other icon libraries

I agree with your analyze. I have come up with the same conclusion. As I was saying, I don't think that we should worry about that tool. It could be a devDependency or implemented here, that don't make much difference.
That's not our end goal. Our end goal is to published svg icons that can be used with material-ui.

By the way, I think that the generated icons should be stored in git. That would make thing simpler for using them with the documentation. Otherwise, we could be using Lerna to link them when developing.

oliviertassinari · 2017-02-10T22:59:36Z

packages/icon-builder/package.json

+  "name": "material-ui-icon-builder",
+  "version": "0.2.0-alpha.1",
+  "description": "Material Design Svg Icons converted to Material-UI React components.",
+  "author": "Hai Nguyen",


Material-UI Team?

oliviertassinari · 2017-02-10T23:00:00Z

packages/icon-builder/package.json

  "devDependencies": {
-    "babel-cli": "^6.6.5",
-    "grunt": "^0.4.5",
+    "babel-cli": "^6.0.0",


Don't we miss a yarn.lock file?

I have no idea!

What is it?

We used yarn over npm to install our dependencies on cicleci. I can take care of that.

oliviertassinari · 2017-02-10T23:01:13Z

packages/icon-builder/package.json

  },
-  "peerDependencies": {},


Don't we need a peer dependency on material-ui rather than a dependency?

We have one: https://github.com/mbrookes/material-ui/blob/ad45aa88cfab602da1135a5b9f2923916248ba63/packages/icon-builder/copy-files.js#L64

My bad 👍 .

oliviertassinari · 2017-02-10T23:03:14Z

packages/icon-builder/test/index.js

@@ -120,14 +118,14 @@ describe('--svg-dir, --innerPath, --fileSuffix', function() {
  });

  after(function() {
-    temp.cleanupSync();
+    // temp.cleanupSync();
  });

  it('script outputs to directory', function(done) {
    builder.main(options, function() {
      assert.ok(fs.lstatSync(tempPath).isDirectory());


I hate assert.ok. I don't think that truthy is good enough for tests. We need precision.
If you don't mind:

assert.strictEqual(fs.lstatSync(tempPath).isDirectory(), true);

oliviertassinari · 2017-02-10T23:05:40Z

packages/icon-builder/test/index.js

@@ -186,15 +184,14 @@ describe('--mui-require', function() {
      var relativeOptions = _.extend({}, options, { muiRequire: 'relative' });


Ohh var, it's some old school Javascript 🙈 .

Haha, that's what I thought! But I wasn't going to start battle at this point.

There's lots about this package that is "old-school" (two-year-old school, crazy isn't it?!)

oliviertassinari · 2017-02-10T23:06:26Z

packages/icon-builder/test/index.js

@@ -120,14 +118,14 @@ describe('--svg-dir, --innerPath, --fileSuffix', function() {
  });

  after(function() {
-    temp.cleanupSync();
+    // temp.cleanupSync();


I don't know what it is for, but we can either remove it or uncomment it.

It shouldn't be commented out - I was trying to view any artifacts from a failing test. I'll fix it.

mbrookes · 2017-02-11T19:42:27Z

Fixed the tests.

mbrookes · 2017-02-12T15:32:03Z

@oliviertassinari I think this is done, other than:

Agreeing on the generated MD icons package name (currently mui-icons, put perhaps mui-md-icons?
Agreeing to disagree on the directory name. 😆

oliviertassinari · 2017-02-12T15:40:30Z

Agreeing to disagree on the directory name.

That's a good one. What's wrong with following the packages convention?

They all follow two patterns:

The name of the folder is the name of the published package.
The package names are all prefixed by the repository's name.

mbrookes · 2017-02-12T16:17:40Z

Well then perhaps we have the npm build script generate ../packages/mui-icons (or whatever we end up calling it) rather than ./build?

mbrookes · 2017-02-13T22:45:12Z

If there's no additional feedback, lets go ahead and merge - the important work is done. (Although it would be nice to get some feedback on the target package name).

oliviertassinari · 2017-02-13T22:47:31Z

lets go ahead and merge - the important work is done.

👍 , we can agree on the name later.

mbrookes added the next label Feb 10, 2017

muibot added the PR: Needs Review label Feb 10, 2017

[icon-builder] Update to generate standalone package

ad45aa8

oliviertassinari requested changes Feb 10, 2017

View reviewed changes

oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Feb 10, 2017

Code review fixes

f1f1c09

muibot added the PR: Needs Review label Feb 11, 2017

oliviertassinari mentioned this pull request Feb 11, 2017

[docs] Add a Drawer section #6113

Merged

Update tests

e9d1d7a

mbrookes removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Feb 11, 2017

Change default output to PascalCase, update README

77cae63

mbrookes added PR: Review Accepted and removed PR: Needs Review labels Feb 14, 2017

mbrookes merged commit f3c8ba7 into mui:next Feb 14, 2017

mbrookes deleted the icon-builder branch February 14, 2017 00:03

oliviertassinari mentioned this pull request Feb 14, 2017

[core] Add exports to index.js for inclusion in webpack bundle #6144

Merged

zannager added the package: icons Specific to @mui/icons label Mar 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[icon-builder] Update to generate standalone package #6104

[icon-builder] Update to generate standalone package #6104

mbrookes commented Feb 10, 2017 •

edited

Loading

mbrookes commented Feb 10, 2017 •

edited

Loading

oliviertassinari left a comment

oliviertassinari Feb 10, 2017

mbrookes Feb 11, 2017

oliviertassinari Feb 10, 2017

mbrookes Feb 10, 2017

oliviertassinari Feb 11, 2017 •

edited

Loading

mbrookes Feb 11, 2017 •

edited

Loading

oliviertassinari Feb 11, 2017

oliviertassinari Feb 10, 2017

mbrookes Feb 11, 2017

oliviertassinari Feb 10, 2017

mbrookes Feb 11, 2017

oliviertassinari Feb 11, 2017

oliviertassinari Feb 10, 2017 •

edited

Loading

mbrookes Feb 11, 2017 •

edited

Loading

oliviertassinari Feb 11, 2017

oliviertassinari Feb 10, 2017

oliviertassinari Feb 10, 2017

mbrookes Feb 11, 2017

oliviertassinari Feb 10, 2017

mbrookes Feb 10, 2017

mbrookes commented Feb 11, 2017

mbrookes commented Feb 12, 2017

oliviertassinari commented Feb 12, 2017 •

edited

Loading

mbrookes commented Feb 12, 2017

mbrookes commented Feb 13, 2017

oliviertassinari commented Feb 13, 2017

		@@ -186,15 +184,14 @@ describe('--mui-require', function() {
		var relativeOptions = _.extend({}, options, { muiRequire: 'relative' });

[icon-builder] Update to generate standalone package #6104

[icon-builder] Update to generate standalone package #6104

Conversation

mbrookes commented Feb 10, 2017 • edited Loading

mbrookes commented Feb 10, 2017 • edited Loading

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Feb 11, 2017 • edited Loading

Choose a reason for hiding this comment

mbrookes Feb 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

mbrookes Feb 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrookes commented Feb 11, 2017

mbrookes commented Feb 12, 2017

oliviertassinari commented Feb 12, 2017 • edited Loading

mbrookes commented Feb 12, 2017

mbrookes commented Feb 13, 2017

oliviertassinari commented Feb 13, 2017

mbrookes commented Feb 10, 2017 •

edited

Loading

mbrookes commented Feb 10, 2017 •

edited

Loading

oliviertassinari Feb 11, 2017 •

edited

Loading

mbrookes Feb 11, 2017 •

edited

Loading

oliviertassinari Feb 10, 2017 •

edited

Loading

mbrookes Feb 11, 2017 •

edited

Loading

oliviertassinari commented Feb 12, 2017 •

edited

Loading