-
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
Ming's Work on Personal API #32
base: master
Are you sure you want to change the base?
Conversation
public/scripts/app.js
Outdated
// }) | ||
// .catch(function(err){ | ||
// console.log(err); | ||
// }); |
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.
Your comments are great! It makes it really easy to understand what you are doing. Keep it up!
},{ | ||
_id: 4, | ||
name: "Underwater", | ||
image_link: "/videos/underwater.mp4", | ||
link: "/videos/underwater.mp4", | ||
} |
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.
I see you have these videos here, but I am not sure where to find them on your API doc.
Overall, this looks awesome. The only things that I found that needed improvement were things could be fixed with more time to work on the project. |
@@ -0,0 +1,12 @@ | |||
// removeSeed.js | |||
// used to clear out data in existing db mainly |
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.
Are you using this file? It doesn't look like you're exporting anything here?
public/styles/styles.css
Outdated
background-color: skyblue; /* Sanity Check! */ | ||
background-color: rgb(239, 239, 239); /* Sanity Check! */ | ||
margin: 0; | ||
|
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 guys have been writing code long enough now where we're going to start looking at your writing style and not just the code itself. Going forward be mindful of extra spaces in these blocks (code between {}) and between blocks.
$(document).ready(function(){ | ||
|
||
main(); |
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.
Great use of comments!
Great job, Ming. I thought it was fun idea to add a seed button! Site looks good and it's fully CRUDable. |
Leave comments for anything you have in mind about my work.