benm
benm copied to clipboard
Gulp
Added delete functionality so that the gulpjs version can send delete requests. I added all the delete functionality from the original gruntjs version the same way, the only thing I had to change to get it to work properly in this version was in client/src/views/contact_details.js was the remove() and destroy() logic.
this.model.destroy({
success: function(model, response) {
window.App.data.contacts.remove(this.model);
}
});
This is my first time doing something like this, so I hope I did it right. Basically I prefer your gulpjs version and when testing it out I couldn't make delete requests and saw you didn't add them. Is there a reason for this?
It deletes the model perfectly fine on my end I hope I implemented it properly, it's basically the same as your original, the only problem I was having is when remove(); was called before destroy(); I was getting url errors. Adding it to the success works wonders.
Awesome work! This looks great and I appreciate the PR for it! If I might make a few tiny suggestions:
- Might make sense to just submit this PR to the master branch instead of gulp. This way everyone benefits and then you can just rebase against the updated master while you still use gulp (since theres nothing specifically regarding gulp in this changeset).
- You might hate me for this, as Im asking for more work, but you might want to include additional tests for teh new code you added. No time like the present to get into the habit?
- Nice work with the commit history!
But again I really do appreciate the extra work you did! If I on don't hear back I will probably just close the PR and submit it as a separate PR against master.
Okay, I take that as a challenge, I haven't done much (well any) testing but you're right I need to make it a habit, I need to learn how to use those tools (like jasmine and the ones you use). "No time like the present to get into the habit?" You're absolutely right! I'm still learning the ropes around here on github I usually leech on projects, but it's time to help collaborate and I'll do my research on how I can follow that criteria you listed above.
I need to better my understanding with marionette so I'll be reading as much as possible on that today and when I have some time I'll submit this PR to the master and try to include additional tests. Also I've been working on using this with socket.io and I just need to add backbone.iobind to really get some real time action going, hopefully that goes well and I can share.
Awesome! To get started I posted an article on testing node at http://kroltech.com that should help. Thanks again for contributing!
Awesome thanks for writing that tut and providing the link. Ill have to check it out sometime today. Got a lot on my plate with classes and projects, I haven't forgotten about your suggestions though.