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

Gulp #15

Open
wants to merge 4 commits into
base: gulp
Choose a base branch
from
Open

Gulp #15

wants to merge 4 commits into from

Conversation

bluematter
Copy link

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

visumdesignz added 4 commits March 14, 2014 04:41
Added a way to destroy() the model inside the gulp.js branch version.
Updated this template to add a button so the deleteContact() method would work. https://github.com/visumdesignz/benm/blob/gulp/client/src/views/contact_details.js
Added a delete route so that deleteContact() inside client/src/views/contact_details.js would send a delete request.
Added delete so that I could get this gulp version to send delete requests.
@bluematter
Copy link
Author

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.

@shorttompkins
Copy link
Owner

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.

@bluematter
Copy link
Author

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.

@shorttompkins
Copy link
Owner

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

@bluematter
Copy link
Author

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.

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