-
Notifications
You must be signed in to change notification settings - Fork 9
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
Hello. I decided to solve Demo task ExpressBoilerplate. #5
base: master
Are you sure you want to change the base?
Conversation
# Conflicts: # app/core/views/_components/html.pug
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.
Thank you @vramaniuk -In general the work/code is good. Just a couple of questions from my side. Please shoot us a message on UpWork so we know who @vramaniuk (you) is on that platform.
@@ -16,6 +16,8 @@ module.exports = (grunt) => { | |||
|
|||
grunt.initConfig(tasks); | |||
grunt.registerTask('build', ['cssmin', 'uglify']); | |||
grunt.registerTask('watch', ['watch']); | |||
grunt.loadNpmTasks('grunt-contrib-watch'); |
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 need to have this here, as it is already in the drop-in task.
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.
But without it here command 'grunt watch' doesn't work at me correctly.
And official documentation (https://github.com/gruntjs/grunt-contrib-watch) says
npm install grunt-contrib-watch --save-dev
Once the plugin has been installed, it may be enabled inside your Gruntfile with this line of JavaScript:
grunt.loadNpmTasks('grunt-contrib-watch');
I'll better remove it from file /grunt/watch.task.js
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 meant this line: grunt.loadNpmTasks('grunt-contrib-watch');
which is already loaded in the drop-in task.
app/core/controllers/cities.js
Outdated
} catch (error) { | ||
return next(error); | ||
} | ||
}, | ||
|
||
read: async (req, res) => { | ||
read: async (req, res, next) => { |
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 logic in this method points to it being a DELETE
. ;)
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 was my first solution for DELETE logic that I forgot to remove after using $.ajax.
So the form does not support the delete method, I implemented POST method with params, but it is not optimal.
Now I'm removing this solution.
await user.remove(); | ||
return res.status(200).end(); | ||
return res.status(200) |
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.
Response for a DELETE
method should be consistent throughout the API. In this case the response is not the same as for DELETE /users
.
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 did not change anything significant here, the editor optimized the markup of the code
return res.status(200)
.end();
Only in app/core/controllers/users.js we are adding in request string 'success' , so now I'll add it here (in app/core/controllers/users.js).
app/core/views/_components/html.pug
Outdated
@@ -9,9 +9,12 @@ mixin head() | |||
meta(http-equiv='X-UA-Compatible', content='IE=Edge') | |||
meta(content='width=device-width, initial-scale=1, maximum-scale=1, user-scalable=no', name='viewport') | |||
link(href='https://fonts.googleapis.com/css?family=Lato' rel='stylesheet') | |||
link(href='/css/home.min.css', rel='stylesheet') |
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 need for this, the build
logic generates a single .css.min
file from /source/css/
.
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.
Now only
link(href='/css/boilerplate.min.css', rel='stylesheet')
app/core/views/home/index.pug
Outdated
@@ -5,7 +5,7 @@ include ../_components/html | |||
+body() | |||
.container | |||
h3 Express boilerplate | |||
p= message | |||
p=message |
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.
Why was the space removed 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.
You are right. It was not necessary
app/core/views/_components/html.pug
Outdated
link(href='/css/boilerplate.min.css', rel='stylesheet') | ||
title Express boilerplate | ||
|
||
mixin body() | ||
body | ||
block | ||
script(src='https://code.jquery.com/jquery-3.3.1.min.js', integrity='sha256-FgpCb/KJQlLNfOu91ta32o/NMZxltwRo8QtmkMRdAu8=', crossorigin='anonymous') | ||
script(src='/js/aj_logic.min.js', type='text/javascript') |
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 should be, by default, /js/boilerplate.min.js
as that is what the build logic will output.
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.
OK
Now script(src='/js/boilerplate.min.js', type='text/javascript')
Hi Phillip, |
I created forms in their respective views (/app/core/views/) that allow you to create and delete users and cities.
Also for implementing delete logic (/app/core/controllers/[users.js,cities.js]) files have been changed.
Update cities logic implemented (/app/core/controllers/cities.js, /app/core/routes/cities.js )
Logger has been added in file (/app/index.js)
For DELETE and UPDATE logic "jQuery ajax" has been used.