-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Node revamp: Controllers Lesson #27695
Node revamp: Controllers Lesson #27695
Conversation
4bd9aab
to
f17bd6d
Compare
f17bd6d
to
0b67561
Compare
bec9a1a
to
f449a83
Compare
0bd8e04
to
0cc9f09
Compare
0cc9f09
to
bc5e301
Compare
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.
Mainly wording/formatting suggestions. It's a big lesson and I've not quite got the time to dive deeper into the rest of it, but will aim to do so at a later point.
Co-authored-by: MaoShizhong <[email protected]>
776aaea
to
48196ad
Compare
Thanks @MaoShizhong I appreciate it! |
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.
Again, mainly nits and suggestions.
Content-wise, fabulous lesson 🤌
Some general things that likely should be changed throughout the file:
- End list items with a period to match most other lessons.
- Capitalise Express, as opposed to express.
- Change "middleware"/"middlewares" to "middleware function" and "middleware functions" respectively, to follow the terminology used by Express itself. The few times Express use "middleware" without "function(s)" after it, it's referring to the general concept of middleware. For example, "application-level middleware" (referring the the middleware functions that are application-level as a whole" or "types of middleware", again being a more "conceptual" use of the word. Whenever it refers to actual functions, it uses "middleware function".
Co-authored-by: MaoShizhong <[email protected]>
Co-authored-by: MaoShizhong <[email protected]>
Resolved all unresolved comments, let me know if there was a mistake. Thanks |
Co-authored-by: MaoShizhong <[email protected]>
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.
@fcasibu this is a chonky lesson 🔥
Particularly love the error handling section. Super cool that it goes through custom errors as well.
Just left a couple of comments below, allow me more time later to dig into the content and code examples.
Co-authored-by: 01zulfi <[email protected]>
cc01fa5
to
28e1566
Compare
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.
some final comments below
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.
lgtm 🚀🚀 🚀
Because
To add a new lesson about Controllers in the MVC pattern for the node revamp
This PR
nodeJS/express/controllers.md
Issue
Closes #27615
Additional Information
Pull Request Requirements
location of change: brief description of change
format, e.g.Intro to HTML and CSS lesson: Fix link text
Because
section summarizes the reason for this PRThis PR
section has a bullet point list describing the changes in this PRIssue
section