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

Icon Builder: handle user-specified SVG collections (not just material-ui-icon) #1057

Merged
merged 4 commits into from
Jul 7, 2015

Conversation

tony
Copy link
Contributor

@tony tony commented Jul 7, 2015

There's a limitation with icon builder hard coded to only build SvgIcon for material-ui-icon.

The scope of this is to expand functionality of icon builder to allow building of arbitrary SVG's, while not adding any additional dependencies.

Some questions:

  • What's the outlook on the use of icon-builder? Is it going to be a major part of building releases in upcoming versions?
  • Is there a need to build utility scripts for any other things?
  • If icon builder is an essential part of the project, is it ok to add a dependency or two for icon-builder (like optimist).
  • Anything inside of here I forget? I'm still testing it out and will follow up too.

Changes

  • Use path.join on directories
  • Accept Environmental arguments:
    OUTPUT_DIR - output directory for jsx components
    SVG_DIR - target directory of svg files
    INNER_DIR - for "reaching into" build directories in
    icon libraries
  • Help dialog with -h or --help
  • build.js --mui-icon-opts to automatically build for
    mui-icon-opts
  • Replace wholesale try-catch where possible for debugging,
    check if file exists / is directory when iterating through
    folders.
  • Use "dir" convention for consistency

tony added 2 commits July 7, 2015 15:05
- Use path.join on directories
- Accept Environmental arguments:
    OUTPUT_DIR - output directory for jsx components
    SVG_DIR - target directory of svg files
    INNER_DIR - for "reaching into" build directories in
    icon libraries
- Help dialog with -h or --help
- build.js --mui-icon-opts to automatically build for
  mui-icon-opts
- Replace wholesale try-catch where possible for debugging,
  check if file exists / is directory when iterating through
  folders.
- Use "dir" convention for consistency
@tony
Copy link
Contributor Author

tony commented Jul 7, 2015

Went ahead and added argument parsing to it with yarg and it's a lot cleaner.

I'm going to double and trickle check the output of material-ui-icons / custom svg sets.

Before I invest too much time in this, @hai-cea: Am I moving in the right direction? Does this this step on anyone's feet that I haven't noticed? I don't see any other conversations about icon builder and building svg icons.

@jkruder
Copy link
Contributor

jkruder commented Jul 7, 2015

@tony It would be nice to generate index.js files for the icons. Some folks have asked about using the icons and there currently is no easy way other than include them one at a time by path (node_modules/material-ui/lib/svg-icons/XXX). We don't want to add them to the main index.js but if the icons had their own master index (and one for each group/theme) that would make it easier for folks to grab the icon(s) they need. @hai-cea What do you think?

@tony
Copy link
Contributor Author

tony commented Jul 7, 2015

@jkruder Can you elaborate more on what you're referring to with index.js ? Do you mean making it so icons could be grabbed via dot-namespaces like normal node modules?

@jkruder
Copy link
Contributor

jkruder commented Jul 7, 2015

@tony If we add index files then we can require the directory and get all of the files exported in the index. https://github.com/callemall/material-ui/tree/master/src/table has an index file in the directory. We could do a require('./table/') (if we were in the src directory) and we would get everything in the index. Utils is another good (probably better) example: https://github.com/callemall/material-ui/tree/master/src/utils. You would do

let Utils = require('./utils/');
let { Dom, Events } = Utils;

@hai-cea
Copy link
Member

hai-cea commented Jul 7, 2015

@tony Thanks for this!
@jkruder Great suggestion - I think it can be addressed in a separate PR.

@tony
Copy link
Contributor Author

tony commented Jul 7, 2015

@jkruder I understand. I'm down for knocking that out since I'm already in it. (in another PR)

@hai-cea : What do you think about this? I do notice a problem, the let SvgIcon in the generated files is relative (which we only want for material-ui project, custom icons need let SvgIcon = require('material-ui').SvgIcon. I'm following up with another commit for that.

…ly. Update mui icon builder to use relative. Make default relative (for people using tool to produce their own icons).
@hai-cea
Copy link
Member

hai-cea commented Jul 7, 2015

@tony That makes sense to me. You may want to use this instead?

let SvgIcon = require('material-ui/lib/svg-icon');

That way the whole library won't get required into someone's project.

@tony
Copy link
Contributor Author

tony commented Jul 7, 2015

@hai-cea Got it. Want me to squash?

@hai-cea
Copy link
Member

hai-cea commented Jul 7, 2015

@tony I think we're good. Thank you!

hai-cea pushed a commit that referenced this pull request Jul 7, 2015
Icon Builder: handle user-specified SVG collections (not just material-ui-icon)
@hai-cea hai-cea merged commit 62b92f4 into mui:master Jul 7, 2015
@tony tony deleted the icon-builder-args branch July 8, 2015 00:00
@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.

4 participants