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

Node revamp: Controllers Lesson #27695

Merged

Conversation

fcasibu
Copy link
Contributor

@fcasibu fcasibu commented Mar 26, 2024

Because

To add a new lesson about Controllers in the MVC pattern for the node revamp

This PR

  • Adds new lesson to nodeJS/express/controllers.md

Issue

Closes #27615

Additional Information

Pull Request Requirements

  • I have thoroughly read and understand The Odin Project Contributing Guide
  • The title of this PR follows the location of change: brief description of change format, e.g. Intro to HTML and CSS lesson: Fix link text
  • The Because section summarizes the reason for this PR
  • The This PR section has a bullet point list describing the changes in this PR
  • If this PR addresses an open issue, it is linked in the Issue section
  • If any lesson files are included in this PR, they have been previewed with the Markdown preview tool to ensure it is formatted correctly
  • If any lesson files are included in this PR, they follow the Layout Style Guide

@github-actions github-actions bot added the Content: NodeJS Involves the NodeJS course label Mar 26, 2024
@fcasibu fcasibu force-pushed the node-revamp/nodejs-controllers-lesson branch 7 times, most recently from 4bd9aab to f17bd6d Compare March 27, 2024 04:01
@fcasibu fcasibu force-pushed the node-revamp/nodejs-controllers-lesson branch from f17bd6d to 0b67561 Compare March 27, 2024 05:11
@fcasibu fcasibu force-pushed the node-revamp/nodejs-controllers-lesson branch from bec9a1a to f449a83 Compare March 27, 2024 05:24
@01zulfi 01zulfi self-requested a review March 27, 2024 09:29
@01zulfi 01zulfi added the Project Node Revamp Issues/PRs related to the Node Revamp project label Mar 27, 2024
@01zulfi 01zulfi self-assigned this Mar 28, 2024
@fcasibu fcasibu force-pushed the node-revamp/nodejs-controllers-lesson branch 4 times, most recently from 0bd8e04 to 0cc9f09 Compare March 31, 2024 07:53
@fcasibu fcasibu force-pushed the node-revamp/nodejs-controllers-lesson branch from 0cc9f09 to bc5e301 Compare April 1, 2024 06:26
Copy link
Contributor

@MaoShizhong MaoShizhong left a 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.

nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
@fcasibu fcasibu force-pushed the node-revamp/nodejs-controllers-lesson branch from 776aaea to 48196ad Compare April 2, 2024 02:35
@fcasibu
Copy link
Contributor Author

fcasibu commented Apr 2, 2024

Thanks @MaoShizhong I appreciate it!

Copy link
Contributor

@MaoShizhong MaoShizhong left a 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".

nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
@fcasibu
Copy link
Contributor Author

fcasibu commented Apr 5, 2024

Resolved all unresolved comments, let me know if there was a mistake. Thanks

Copy link
Member

@01zulfi 01zulfi left a 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.

nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
@fcasibu fcasibu requested a review from 01zulfi April 28, 2024 08:33
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
@fcasibu fcasibu force-pushed the node-revamp/nodejs-controllers-lesson branch from cc01fa5 to 28e1566 Compare May 11, 2024 10:04
Copy link
Member

@01zulfi 01zulfi left a 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

nodeJS/express/controllers.md Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
nodeJS/express/controllers.md Outdated Show resolved Hide resolved
@fcasibu fcasibu requested a review from 01zulfi May 25, 2024 11:24
Copy link
Member

@01zulfi 01zulfi left a comment

Choose a reason for hiding this comment

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

lgtm 🚀🚀 🚀

@01zulfi 01zulfi merged commit 9c55a81 into TheOdinProject:main May 25, 2024
1 of 3 checks passed
@fcasibu fcasibu deleted the node-revamp/nodejs-controllers-lesson branch May 25, 2024 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content: NodeJS Involves the NodeJS course Project Node Revamp Issues/PRs related to the Node Revamp project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Lesson: Controllers
3 participants