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

Chen's work on personal API #27

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

Conversation

achentha
Copy link

@achentha achentha commented Sep 5, 2017

Seeding was working for awhile. But later on it does not quite update in heroku any more.

</head>
<body>

Choose a reason for hiding this comment

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

Watch for indentation in your code

@@ -13,3 +13,98 @@
// console.log("Created new campsite", campsite._id)
// process.exit(); // we're all done! Exit the program.
// })
const db = require("./models");

Choose a reason for hiding this comment

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

indentation needed a bit more here

Copy link

Choose a reason for hiding this comment

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

+1

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, this looks really good. You made it through all the requirements and have reasonable styling. Nice work.

server.js Outdated
{method: "GET", path: "/api/profile", description: "About me"},
{method: "GET", path: "/api/birds", description: "Show all my favorite birds"},
{method: "POST", path: "/api/birds", description: "Add a new favorite bird"},
{method: "PUT", path: "/api/birds", description: "Modify a favorite bird"},
Copy link

Choose a reason for hiding this comment

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

These routes are actually (correctly!) at /api/birds/:id, not at /api/birds as this documentation says.

views/index.html Outdated

<!-- VENDOR SCRIPTS -->
<script src="//ajax.googleapis.com/ajax/libs/jquery/2.2.2/jquery.min.js"></script>
<script src="http://ajax.googleapis.com/ajax/libs/jquery/2.2.2/jquery.min.js"></script>
Copy link

Choose a reason for hiding this comment

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

The fact that you load this over http always, rather than using https, means that your site doesn't work the first time on Heroku, and the user has to manually click "allow unsecure scripts".

@@ -13,3 +13,98 @@
// console.log("Created new campsite", campsite._id)
// process.exit(); // we're all done! Exit the program.
// })
const db = require("./models");
Copy link

Choose a reason for hiding this comment

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

+1

seed.js Outdated
console.log("successfully created birdSeed");
});

process.exit(); //so it does not hang there
Copy link

Choose a reason for hiding this comment

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

Having this at the end here is dangerous! It means that you might exit the process before the data is finished saving. This line should go INSIDE your callback (i.e. line 108/109), so that you only exit the process after the data is saved.

background-size: contain;
}

#bird-form {
Copy link

Choose a reason for hiding this comment

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

empty styling feels like a leftover.

function handelEditBird(e) {
e.preventDefault();
let birdId = $(this).closest(".bird").data("bird-id");
let $bird = $(this).closest(".bird");
Copy link

Choose a reason for hiding this comment

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

These two lines do the same bunch of work, and you could put them in the opposite order to great effect!

let $bird = $(this).closest(".bird");
let birdId = $bird.data("bird-id");


//change the bird name in the span to become input with the value of the current bird name
let birdName = $bird.find(".bird-name").text();
$bird.find(".bird-name").html(`<input class="edit-bird-name" value="${birdName}"></input>`);
Copy link

Choose a reason for hiding this comment

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

similarly, could do less work here:

let $birdName = $bird.find(".bird-name");
let oldName = $birdName.text();
$birdName.html(`...`)

$("[data-bird-id=" + birdId +"]").remove();
renderBird(updatedBird);

$("[data-bird-id=" + birdId + "]")[0].scrollIntoView();
Copy link

Choose a reason for hiding this comment

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

awesome use of this method!

//handling delete button of a bird
$("#birds").on("click", ".delete-bird", handleDeleteBird);

function renderAllBirds(birds) {
Copy link

Choose a reason for hiding this comment

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

this is lovely.

db.Bird.findById(req.params.birdId, function(err, bird){
if (err) {
console.log(`Did not find bird id: ${req.params.birdId} in db`);
return;
Copy link

Choose a reason for hiding this comment

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

You should probably send a response here, not just return from the function; as is, your app.js file would be waiting forever, for a response that would never come, if there were an error. Instead, you could send a 404.

Copy link

Choose a reason for hiding this comment

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

And this goes for all of your error-handling: you should send a response, not just console.log.

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.

3 participants