-
-
Notifications
You must be signed in to change notification settings - Fork 0
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: post snippet #549
feat: post snippet #549
Conversation
Reviewer's Guide by SourceryThis PR implements a new snippet feature that allows embedding HTML content (specifically ads) within blog posts. The implementation includes adding a new Class diagram for the new Snippet componentclassDiagram
class Snippet {
+Snippet(html: string, className: string)
+render(): JSX.Element
}
note for Snippet "New component to render HTML snippets within blog posts"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Hey @duyet - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding HTML sanitization to the Snippet component to prevent potential XSS attacks when rendering arbitrary HTML content.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Review instructions: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -30,6 +31,8 @@ export default function Content({ post }: { post: Post }) { | |||
)} | |||
dangerouslySetInnerHTML={{ __html: post.content || 'No content' }} | |||
/> | |||
|
|||
<Snippet html={post.snippet as string} /> |
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.
suggestion: Consider removing the type assertion as the Snippet component already handles undefined properly
The Post interface already defines snippet as string | undefined, and the Snippet component handles undefined cases. The type assertion isn't necessary and could mask potential type issues.
<Snippet html={post.snippet as string} /> | |
<Snippet html={post.snippet} /> |
Summary by Sourcery
Add support for HTML snippets in blog posts, enabling the integration of elements like ads. Update the content component to render these snippets using a new Snippet component.
New Features:
Enhancements: