-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
NodeJS Controllers: Update lesson to flow with the previous lesson #28665
NodeJS Controllers: Update lesson to flow with the previous lesson #28665
Conversation
96ca621
to
a2c2940
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.
Many thanks for working through the feedback!
Some initial comments - I haven't had the time to look at the error handling section and beyond, so I'll get to those in due course (as well as the Routes lesson changes I mentioned).
Co-authored-by: MaoShizhong <[email protected]>
c238eee
to
a895767
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.
Whew! Amazing work. Just a few more small things and a small merge conflict due to #28858 but otherwise I'm happy to merge this along with the Routes changes PR when that's also approved.
nodeJS/express/controllers.md
Outdated
const getAuthorById = async (authorId) => { | ||
return authors.find(author => author.id === authorId); | ||
}; |
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.
Looking through the rest of the curriculum, it actually looks like most of the time we use function declarations. Nit but for the sake of consistency, I think the functions in this lesson should be func decs unless they're callbacks defined inline (e.g. asyncHandler callback).
const getAuthorById = async (authorId) => { | |
return authors.find(author => author.id === authorId); | |
}; | |
async function getAuthorById(authorId) { | |
return authors.find(author => author.id === authorId); | |
}; |
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.
done with changing most functions
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.
Amazing work as always 🙏 Just the one grammar nit but otherwise I'm happy with this and will merge together with #28763 when that's approved.
Co-authored-by: MaoShizhong <[email protected]>
Because
Doesn't flow well enough with the previous lesson and includes passive tone in writing.
This PR
Issue
Closes #28530
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