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

Bryan A's Personal API #29

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Bryan A's Personal API #29

wants to merge 17 commits into from

Conversation

balbini
Copy link

@balbini balbini commented Sep 5, 2017

My delete button is able to get the data to remove, but the button itself does not work.
if you are able, please give me some tips on how to get it to work, that would be great.

@@ -2,6 +2,95 @@ console.log("Sanity Check: JS is working!");

$(document).ready(function(){

// your code
// Profile GET Statement
Copy link

Choose a reason for hiding this comment

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

I assume you didnt get a chance to use the profile data?

$.post("/api/heroes", userHero, function(data){
renderHero(data);
});
$(this).trigger("reset");
Copy link

Choose a reason for hiding this comment

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

Nice, I didnt know about the trigger("reset") to clear the input fields

});
// Delete Function
function removeHero(data){
let deletedHero = $(this).parents(".data-hero-id").data("hero._id")
Copy link

Choose a reason for hiding this comment

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

I think it should be let deletedHero = $(this).parents(".data-hero-id").data("hero.-id"). But this is only client side, if you want to delete it in the server, you should first send a delete request to the server passing the id (in this case deletedHero) at url. When the server.js or one of your controllers catches it, you query your database, find that matching id and delete it. The mongoose function that lets you do that is findByIdAndRemove(req.params.id, ... ). Assuming your db is call Hero, you will you will do db.Hero.findByIdAndRemove(req.params.id, .... ). And when you are done, you send back a 200 http code and then delete it on client side.

let heroesHTML = (`
<div class="data-hero-id="${hero._id}">
<div class="col-md-10 col-md-offset-1">
<div class="spacer">
Copy link

Choose a reason for hiding this comment

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

What does the spacer class suppose to do?

views/index.html Outdated
<label for="secret_identity">Secret Identity</label>
<input name="secret_identity" type="boolean" placeholder="Secret Identity?">
<button type="submit" type="button" class="btn btn-primary">Save New Hero</button>
<button type="button" class="btn btn-danger">Cancel</button>
Copy link

Choose a reason for hiding this comment

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

I would do a reindent for the block of code between form to make it easier to read. Not sure if its github that mess up rendering your code online.

Copy link

@mnfmnfm mnfmnfm left a comment

Choose a reason for hiding this comment

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

Overall, a good attempt (especially given your overheating computer). You have a couple of bugs with how delete is working that reveal some underlying gaps in understanding, specifically around when to make the AJAX request for delete (after button is clicked) and how to pass a callback function as an argument.

app.get("/api/profile", controllers.profile.index);
app.get("/api/heroes", controllers.heroController.index);
app.post("/api/heroes", controllers.heroController.create);
app.delete("/api/heroes/:Id", controllers.heroController.delete)
Copy link

Choose a reason for hiding this comment

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

A little weird to use capital letters for your params (although it obviously works)

}
]

// superHero.forEach(function(new_hero){
Copy link

Choose a reason for hiding this comment

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

This code looks helpful! Sad that it's commented out, so your seed file doesn't get to do anything.

$.ajax({
method: "GET",
url:"/api/profile"
})
Copy link

Choose a reason for hiding this comment

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

It's weird that you make this AJAX call without it doing anything; you could remove this call.



// Function to show seeded DB
function displayHero(heroes){
Copy link

Choose a reason for hiding this comment

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

This function is named displayHero singular, but its parameter is heroes plural; this is one of those things that works, but will trip you up if you ever forget that this function is named singularly but takes plural arguments.

$("#hero-list").on("click", ".btn-danger", removeHero());
// DELETE Ajax Call
$.ajax({
url: "/api/heroes/:Id" + removeHero(),
Copy link

Choose a reason for hiding this comment

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

This AJAX call is in the middle of your code, not actually attached to the delete button at all, and it uses the literal string ":Id" instead of the actual ID of a hero.

$(this).trigger("reset");
});
// Delete button actions on click event
$("#hero-list").on("click", ".btn-danger", removeHero());
Copy link

Choose a reason for hiding this comment

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

You try to pass removeHero as an argument here, but call it instead... this is the difference between getTwo (a function) and getTwo() (a number). You want to pass the function, removeHero, as the argument, because that's the code you want executed after the button is clicked.

});
// Delete Function
function removeHero(data){
let deletedHero = $(this).parents(".data-hero-id").data(".hero.-id")
Copy link

Choose a reason for hiding this comment

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

I think it's just .data("hero-id")

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.

4 participants