-
Notifications
You must be signed in to change notification settings - Fork 131
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
Feat/admin domains #124
Feat/admin domains #124
Conversation
}); | ||
|
||
// this will assign rendered html to ctx.body | ||
await ctx.render('admin/domains/_table', { |
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 is an anti-pattern and bad practice for code readability to assume users know that calling ctx.render
assigns to ctx.body
.
Instead you should write it out here as ctx.body = await ctx.render(...)
.
I think all we'd have to do is add a return html
here to koa-views
to support this @ https://github.com/queckezz/koa-views/blob/master/src/index.js#L72
I'm an author on koa-views
on npm I think so once you submit a PR I can merge and release with semver major bump.
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.
We will also need to change this everywhere else we do this practice, as it's an anti-pattern to write in a way that's not readable to newbies.
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.
See more context here https://titanism.slack.com/archives/C02FU8YN0L8/p1637614211010500
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.
waiting on this to be accepted.
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.
See comments, thank you!
5ef534a
to
99f513c
Compare
99f513c
to
372e17d
Compare
No description provided.