-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Jevell's personal API #36
base: master
Are you sure you want to change the base?
Conversation
|
||
}; | ||
|
||
|
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.
Lots of spaces... are these needed?
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.
No they're not Carlynn. Jevell tighten this code up!
console.log('id', shopId); | ||
$.ajax ({ | ||
method: 'DELETE', | ||
url: '/api/shops/' + shopId, |
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.
Try editing the url. Instead of shopId use $(this).attr('data-id')
} | ||
|
||
|
||
// app.get('/api', function apiIndex(req, res) { |
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.
File looks good!
Once a file is complete, remove unused pseudocode and delete extra spaces.
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 agree with Carlynn.
} | ||
|
||
|
||
// app.get('/api', function apiIndex(req, res) { |
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 agree with Carlynn.
module.exports = { | ||
api: require('./apiController'), | ||
shops: require('./shopsController'), | ||
// albumsSongs: require('./albumsSongscontroller') |
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.
Sadness. Make sure delete commented out code - especially if it's pasted from another project.
res.json(deleteShop); | ||
}); | ||
} | ||
|
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 code style is just as important as the code itself. Moving forward we're going to be more focused on ensuring you're writing with the industry best practices. This many spaces between functions is not best practice.
@@ -0,0 +1,21 @@ | |||
// shopsSongsController |
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 don't think you use this file - there is no route to it? Maybe this should be deleted. Also again, if you're cutting and pasting from another project make sure you remove the comments (or change them) in the current project.
|
||
}; | ||
|
||
|
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.
No they're not Carlynn. Jevell tighten this code up!
Overall I think you did a really great job. You site looks good, just moving forward really start focusing on how your code looks as well. Delete commented code, make sure you have some descriptive comments, clean up multiple spaces, etc. |
buttons are getting logged in console but not working on backend