benm icon indicating copy to clipboard operation
benm copied to clipboard

Gulp

Open bluematter opened this issue 10 years ago • 5 comments

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);
  }
});

bluematter avatar Mar 14 '14 09:03 bluematter

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.

bluematter avatar Mar 14 '14 09:03 bluematter

Awesome work! This looks great and I appreciate the PR for it! If I might make a few tiny suggestions:

  1. 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).
  2. 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?
  3. 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.

shorttompkins avatar Mar 14 '14 12:03 shorttompkins

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.

bluematter avatar Mar 15 '14 10:03 bluematter

Awesome! To get started I posted an article on testing node at http://kroltech.com that should help. Thanks again for contributing!

shorttompkins avatar Mar 15 '14 13:03 shorttompkins

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.

bluematter avatar Mar 17 '14 18:03 bluematter