-
Notifications
You must be signed in to change notification settings - Fork 54
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
base: master
Are you sure you want to change the base?
Conversation
…dating the server
</head> | ||
<body> | ||
|
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this 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"}, |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
public/styles/styles.css
Outdated
background-size: contain; | ||
} | ||
|
||
#bird-form { |
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
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>`); |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Seeding was working for awhile. But later on it does not quite update in heroku any more.