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

Add TSDoc, style and className props #543

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

DerGoogler
Copy link

I added some TSDocs to the Giscus type definition. I also added style and className props for inline styling. Class name is useful for styled components

Feel free to make/open requests :D

@vercel
Copy link

vercel bot commented Sep 3, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
giscus-component ✅ Ready (Inspect) Visit Preview Sep 12, 2022 at 1:16PM (UTC)

Copy link
Member

@laymonage laymonage left a comment

Choose a reason for hiding this comment

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

Thanks for the effort, though I'm not sure if this would work. Also, if we're adding docs, I'd love it if it's added to all of the components instead of just doing this for React. Not sure if there's a way to not duplicate the docs across different files, it would be very nice if it's possible to use a single source for the docs...

@@ -30,6 +32,8 @@ export default function Giscus({

return (
<giscus-widget
style={style}
className={className}
Copy link
Member

Choose a reason for hiding this comment

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

Does this work? It's a custom element so I doubt this could just work as-is, since the element doesn't explicitly accept style and class attributes.

id?: string;
host?: string;
repo: `${string}/${string}`;
repoid: string;
category?: string;
categoryid?: string;
categoryid: string;
Copy link
Member

Choose a reason for hiding this comment

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

This isn't required if the chosen mapping is number.

@DerGoogler
Copy link
Author

@laymonage, I can check this the days, but I don't know so much over Angular and Vue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants