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

Ming's Work on Personal API #32

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

Conversation

rollba14
Copy link

@rollba14 rollba14 commented Sep 5, 2017

Leave comments for anything you have in mind about my work.

// })
// .catch(function(err){
// console.log(err);
// });
Copy link

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",
}
Copy link

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.

@balbini
Copy link

balbini commented Sep 5, 2017

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.
Your functionality is solid and everything works the way that I would expect. The style of the site could be improved; when you go to the My Profile Page, the images are different sizes and/or focus. The buttons in the upper right overlap on some images as well. I really like the code you wrote, as well as the comments. It was very clear what you were trying to do throughout the project, well done.

@@ -0,0 +1,12 @@
// removeSeed.js
// used to clear out data in existing db mainly
Copy link

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?

background-color: skyblue; /* Sanity Check! */
background-color: rgb(239, 239, 239); /* Sanity Check! */
margin: 0;

Copy link

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();
Copy link

Choose a reason for hiding this comment

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

Great use of comments!

@spragala
Copy link

spragala commented Sep 6, 2017

Great job, Ming. I thought it was fun idea to add a seed button! Site looks good and it's fully CRUDable.

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