Cleanup and refactor a little bit? about font-awesome-svg-png HOT 12 CLOSED

Hi, it's alive and kicking. I would be happy to work with you to achieve these goals.

from font-awesome-svg-png.

Cool, will look at it more on Monday :)

from font-awesome-svg-png.

@dominykas The existing code is really a mess. The generated SVG files actually change depending on whether you are generating PNG files due to some side effects…

Since I don't care about PNG, I wrote a simpler script in ES6 at https://github.com/riobard/font-awesome-svg/blob/master/build.js It does not download icons.yml at all.

Let me know if it would be useful to clean up here.

from font-awesome-svg-png.

Yeah, I admit the whole thing was put together quite quickly and pushed to Github in hope it's useful to someone. For me the most common use case turned out to be pixel perfect pngs.

from font-awesome-svg-png.

@dominykas @riobard I have just updated the package to utilize promises, split up some parts of the code, move out sprites to a separate function, fix the side effects problem and so on ... I doubt I will have time next week to split the generation code to an API but I would welcome such PR.

from font-awesome-svg-png.

@Rush Great to see the clean up and thanks a lot for the initial work to get it started :)

However, your viewBox calculation is still wrong. See cc-visa.png for example. The icon is not a square and your method cut it off at the edge. There are many other instances like this. Please refer to my implementation for the correct method https://github.com/riobard/font-awesome-svg/blob/master/build.js#L35

from font-awesome-svg-png.

Yeah, looks kind of blurry :-( https://raw.githubusercontent.com/encharm/Font-Awesome-SVG-PNG/master/black/png/16/credit-card.png

from font-awesome-svg-png.

@riobard neat trick with the less variable parsing - totally copy/pasted that: insidewarehouse@c185a18

Only yours needs two minor tweaks:

1. regex should include numbers (to match html5, css3) - I went for \w
2. instead of trying to read from node_modules you should use require.resolve as that will find the installed module correctly, using node's default logic (be it global or installed with a parent package or whatever)

I then compared the generated list with what was being read from the yaml, and found that it's actually the same plus it has all the aliases that were not taken into account.

Will PR later.

from font-awesome-svg-png.

@dominykas Awesome and thanks for the tweaks! 👍 Can't wait for the PR to come.

I believe Font Awesome generates its variables.less from the YAML file by flattening the alias list per icon anyway. The drawback with parsing variables.less is that there's no way to know which alias is the canonical one. I'm totally fine with that though :D

from font-awesome-svg-png.

OK, I think I'm done for now - there's plenty that can still be done, but I've split the code up enough so that I can find my way around it so that I could extract the primary generate() function (can be used separately, e.g. inside a grunt task). I've also made this depend on font-awesome package and I've added the --dest option for output folder. This is now good enough for our own internal use :)

Please review/comment/merge/publish :)

Once that's out - I'll go PR the grunt tasks... Going by node's philosophy, they shouldn't be built into this package.

from font-awesome-svg-png.

@dominykas merged, many thanks

from font-awesome-svg-png.

Shall we close this issue then?

from font-awesome-svg-png.