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

#1127 #1126 #1125 #1139 Icon builder tests, grunt, templating #1130

Merged
merged 4 commits into from
Jul 16, 2015

Conversation

tony
Copy link
Contributor

@tony tony commented Jul 11, 2015

All prior behavior works as intended. Plus:

  • main() now accepts a opts, this is used inside of test/index.js and makes the builder usable as an API in user made scripts.
  • Wrap argv in a function + require..main === module so it's not ran when build.js is imported as a library
  • Add a new argument --renameFilter that can pass in module/js file to rename stuff.
  • Adds grunt
  • Add grunt watch to rerun tests on save
  • grunt mochaTest to run mocha tests
  • Tests
    • absolute / relative styles of including library (relative is used on the SvgIcon's in the project)
    • against a an excerpt of a non-mui SVG icon set (game icons)
    • Output directory
    • chai asserts / temp directories
    • fixtures for mui/game icons (also has a variable to toggle using the full library)
    • basic function tests
  • Clean up old comments / unnecessary code

Helpful for #1064

@tony tony changed the title #1127 #1126 #1125 Icon builder tests #1127 #1126 #1125 Icon builder tests, grunt, templating Jul 11, 2015
@tony
Copy link
Contributor Author

tony commented Jul 11, 2015

Finishing this tomorrow.

@tony tony changed the title #1127 #1126 #1125 Icon builder tests, grunt, templating #1127 #1126 #1125 #1139 Icon builder tests, grunt, templating Jul 12, 2015
@tony
Copy link
Contributor Author

tony commented Jul 12, 2015

@hai-cea : Ok, updated the original post with the changes. This preserves current functionality and adds test to back it. build.js now works as a module or script, so if a developer has a custom need they can pick what they want.

What do you think?

@tony tony force-pushed the icon-builder-test branch 3 times, most recently from 1dd6409 to a8ac948 Compare July 13, 2015 12:51
@hai-cea
Copy link
Member

hai-cea commented Jul 13, 2015

@tony I'm trying to run the default build and am getting some weird results. Are you seeing the same?

//Running with:
npm run build

Here's the JSX that it's currently generating:

let React = require('react');
let SvgIcon = require('../../svg-icon');

;

let ActionAccessibility = React.createClass({

  render() {
    return (
      <path d="M12 2c1.1 0 2 .9 2 2s-.9 2-2 2-2-.9-2-2 .9-2 2-2zm9 7h-6v13h-2v-6h-2v6H9V9H3V7h18v2z"/>
    );
  }

});

module.exports = ActionAccessibility;

Here's what it should generate:

let React = require('react');
let SvgIcon = require('../../svg-icon');

let ActionAccessibility = React.createClass({

  render() {
    return (
      <SvgIcon {...this.props}>
        <path d="M12 2c1.1 0 2 .9 2 2s-.9 2-2 2-2-.9-2-2 .9-2 2-2zm9 7h-6v13h-2v-6h-2v6H9V9H3V7h18v2z"/>
      </SvgIcon>
    );
  }

});

module.exports = ActionAccessibility;

@tony tony force-pushed the icon-builder-test branch from 002ac89 to 005d517 Compare July 14, 2015 18:25
- Add grunt, mocha.
- Modularize functions in build.
- Tests for functions and types.
- Grunt watch for rerunning tests on file save.
- Wrap argv in main() + only run when executing as a script.
- main() now accepts parameterized object literal for simulating args.
- mui#1139 split file renaming into module / argument.
- Tests + Fixture for result output.
@tony tony force-pushed the icon-builder-test branch from 005d517 to 9782c5a Compare July 14, 2015 18:25
@tony
Copy link
Contributor Author

tony commented Jul 14, 2015

@hai-cea: Good catch,

Fixed template and added a test for output.

- Search through files via node-glob
- Use _.defaults for default options
- Custom filters are now more flexible, output relative
  to options.outputDir / --output-dir
- mui-icons no longer needs --file-suffix, _24px.svg -> .jsx
  happens in filter.
- pascalCase now capitalizes first letter of recursive directories
  and dashes.
@tony
Copy link
Contributor Author

tony commented Jul 16, 2015

The last commit I made simplified things a lot. It handle svg icons for my other icon sets as well as the project's.

  • Passes tests (including test added to verify output on a sample file)
  • Adds node-glob, mkdirp to scan for file matches (instead of the hard-coded way + it can recursively find svg files now)
  • Middleware allows user to rewrite output of files relative to the output directory. By default the output will use the same structure --svgDir / options.svgDir had.
  • Arguments for the functions pass options in via options. Default options that work when using build.js as API / unit testing too.

@hai-cea
Copy link
Member

hai-cea commented Jul 16, 2015

Thanks @tony - I'm getting build error when trying to npm run build

hai-mbpro:icon-builder Hai$ npm run build

> [email protected] prebuild /Users/Hai/GitHub/material-ui/icon-builder
> rm -rf js


> [email protected] build /Users/Hai/GitHub/material-ui/icon-builder
> npm run createMuiIconsJsx && babel --stage 1 ./jsx --out-dir ./js


> [email protected] createMuiIconsJsx /Users/Hai/GitHub/material-ui/icon-builder
> node build.js --output-dir jsx --svg-dir ./node_modules/material-design-icons --glob '/**/*_24px.svg' --mui-require relative --renameFilter ./filters/rename/material-design-icons.js

module.js:338
    throw err;
          ^
Error: Cannot find module 'glob'
    at Function.Module._resolveFilename (module.js:336:15)
    at Function.Module._load (module.js:278:25)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at Object.<anonymous> (/Users/Hai/GitHub/material-ui/icon-builder/build.js:16:12)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Function.Module.runMain (module.js:501:10)


@tony
Copy link
Contributor Author

tony commented Jul 16, 2015

@hai-cea Give it a shot now. It was missing a package.

@hai-cea
Copy link
Member

hai-cea commented Jul 16, 2015

@tony Beautiful - thanks! 👍

hai-cea pushed a commit that referenced this pull request Jul 16, 2015
@hai-cea hai-cea merged commit 0004660 into mui:master Jul 16, 2015
@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.

3 participants