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

Pedro/svg sprites #2

Merged
merged 5 commits into from
Dec 12, 2016
Merged

Pedro/svg sprites #2

merged 5 commits into from
Dec 12, 2016

Conversation

pmpinto
Copy link
Contributor

@pmpinto pmpinto commented Sep 16, 2016

Hey @stkao05!

The SVG sprite task is now on a separate task, through gulpist svg-sprite.
Moved from dr-svg-sprites to the recommended gulp-svg-sprite.

And updated the documentation. Hope you don't mind some styling 😅

Let me know if something needs to be polished! 👍

Pedro Pinto added 2 commits September 16, 2016 18:35
Moves the svg sprite task to a separate task
Updates the documentation accordingly
@stkao05
Copy link
Contributor

stkao05 commented Sep 19, 2016

Hi Pedro,
could you

  • make the svg-sprite task as a separate codes in lib/tasks/svg_sprites.js?
  • revert sprites.js to the state before svg sprite is added?

Thanks

@pmpinto
Copy link
Contributor Author

pmpinto commented Sep 19, 2016

Hey @stkao05!
Sorry about that, I didn't get that was what you wanted from the previous time. Will make these changes right away 👍

@pmpinto
Copy link
Contributor Author

pmpinto commented Sep 20, 2016

There you go @stkao05 🙂

Let me know if I missed something.

@stkao05
Copy link
Contributor

stkao05 commented Sep 21, 2016

Hi Pedro, looks good. Codes work but for the sake of learning I would also like you to look into a last couple change.

  • svg-sprite task didn't comply gulp task API. If you look at the output carefully
gulpist svg-sprite
[14:32:31] Working directory changed to ~/todoist/gulpist/lib
[14:32:33] Using gulpfile ~/todoist/gulpist/lib/gulpist.js
[14:32:33] Starting 'svg-sprite'...
2016-08-21 14:32:33 - info: Created spriter instance
[14:32:33] Finished 'svg-sprite' after 18 ms
2016-08-21 14:32:33 - info: Compiling 8 shapes ...
2016-08-21 14:32:33 - info: Laying out «css» sprite («css» mode)
2016-08-21 14:32:33 - info: Finished «css» sprite compilation
2016-08-21 14:32:33 - info: Created 1 x SVG, 1 x LESS

You will notice that [14:32:33] Finished 'svg-sprite' after 18 ms is printed before result files are generated. This is due to the fact the task code didn't comply with gulp task API. You might want to return a steam. (please see sprite task as an example)

The current code will still run, but if there are other task depending on completion this task, the code will not work incorrectly.

  • gulp-svg-sprite should be in dependencies instead of devDependencies. The difference being thatdevDependencies is the libraries you used while developing gulpist, and dependencies are libraries that needs to be ship with gulpist.

Pedro Pinto added 2 commits November 18, 2016 19:26
Also moves gulp-svg-sprite from devDependencies to dependencies
@pmpinto
Copy link
Contributor Author

pmpinto commented Nov 18, 2016

Hey @stkao05!
This is surely a late reply as I didn't set this a high priority task and only this week found the time to learn more about gulp streams.

It seems to be working properly now. Let me know if there's anything that needs to be fixed 👍

@stkao05 stkao05 merged commit 3a33606 into master Dec 12, 2016
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