nfroidure / svgfont2svgicons

Extract SVG icons from an SVG font
MIT License
15 stars 10 forks source link

Fixed 'cant read property pipe of undefined', added option --namemap #7

Closed M4R7iNP closed 9 years ago

M4R7iNP commented 9 years ago

I got the exception "TypeError: Cannot read property 'pipe' of undefined" which I fixed in this commit.

--namemap is a json file that maps unicode to icon name Useful when reconstructing a svg font without names embedded

Example name map: { "\uF167": "youtube", "\uF16A": "youtube-play", "\uF166": "youtube-square" }

Example usage: node svgfont2svgicons.js --namemap ./fontawesome.json ./fontawesome-webfont.svg ./icons/

nfroidure commented 9 years ago

Thanks for the PR but it comes with no tests :(.

Also, i would have prefer using commander (like i did in the svgicons2svgfont project) instead of manually parsing CLI args. It is way far clearer. https://github.com/nfroidure/svgicons2svgfont/blob/master/bin/svgicons2svgfont.js#L9 https://github.com/nfroidure/svgicons2svgfont/blob/master/tests/cli.mocha.js

Th name mapping system should be in its own file, tested and could be override through an option like it was done for the metadataProvider of svgicons2svgfont: https://github.com/nfroidure/svgicons2svgfont/blob/master/src/metadata.js https://github.com/nfroidure/svgicons2svgfont/blob/master/tests/metadata.mocha.js

The feature is meaningful but i won't merge as is.

nfroidure commented 9 years ago

Also worth noting the name mapping should be done at the API level instead than in the CLI file to be usable either through the API and CLI tool.

M4R7iNP commented 9 years ago

I moved it to api level which, as you said, makes much more sense.

Thanks for the commander tip - I have always found it faster to write a simple argv parser than to compare and find a fitting argument parser.

I have also written a test for it. I planned write it if you found the feature meaningful.

nfroidure commented 9 years ago

Nice work, thanks for the contrib.