Skip to content

Conversation

@blai
Copy link

@blai blai commented Nov 20, 2014

@iamdarrenhall Thanks for sharing this great plugin. I encountered a few bugs, and while trying to resolve them, I throw in a few enhancements too. Unfortunately, I have an auto code prettifier running and reformatted your source (hope you don't mind).

Here are the details of this change:

  1. combined loadTemplate + renderTemplate
  2. stream through all files (sprite.svg, sprite.css, [optional] sprite.html), instead of some coming from stream, some being written by 'fs'
  3. added svgDest option to allow configuration of the output .svg file name + path
  4. changed to use some of the best practices of writting gulp plugin
  5. fixed various bugs (e.g. end the stream)

@frontendbeast
Copy link
Owner

Thanks for the pull request, however maybe you could enter a slightly aggressive tile next time? I'd love to look through your changes but as you've changed the indenting to two spaces instead of four I'm having trouble seeing them. If you could resubmit using the existing indenting that would be great.

@blai
Copy link
Author

blai commented Nov 20, 2014

understood, I will clean up the format later today for review. It was past midnight when I did the first PR, should have do it with a better timing :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be a sync call so the result can be pushed into stream before the callback is triggered.

@blai blai force-pushed the master branch 3 times, most recently from 136fe83 to ea4d1cf Compare November 20, 2014 19:04
@blai blai changed the title Make this plugin actually work! Make this plugin work more similar to gulp-svg-sprite Nov 20, 2014
@blai
Copy link
Author

blai commented Nov 20, 2014

@iamdarrenhall I reverted back the code style manually (as much as I can). Please let me know if you see any issue. I'd recommend setting up a code style guide (or use one of the popular one out there), it was a bit hard to follow some of the spacing in the current version as it has more than one style from line to line. Just my 2 cents.

@blai
Copy link
Author

blai commented Nov 30, 2014

@iamdarrenhall I saw your did some commits that fix some of the same issues I fixed here, have you considered merging this in any time soon?

@frontendbeast
Copy link
Owner

@blai I'll try and look at merging this week. Unfortunately I can only fix project related bugs at work, so that's why I've done a couple that you've already spotted. The rest I have to do in my free time, which I've not had much of lately! Thanks for contributing though :–)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants