-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
[Feature/giscus] add giscus support #536
base: main
Are you sure you want to change the base?
Conversation
(cherry picked from commit f889410)
@sgamerw is attempting to deploy a commit to the Saasify Team on Vercel. A member of the Team first needs to authorize it. |
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected] |
components/PageFooter.tsx
Outdated
repo="giscus/giscus" | ||
repoId="MDEwOlJlcG9zaXRvcnkzNTE5NTgwNTM=" | ||
category="General" | ||
categoryId="MDE4OkRpc2N1c3Npb25DYXRlZ29yeTMyNzk2NTc1" |
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.
Thank you for this PR, but please don't use these as the default, otherwise people who haven't configured the values with their own repo will spam the giscus repo with their discussions.
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.
so, maybe just keep these configuration empty, then tell users how to set these in docs/wiki/readme?
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.
I haven't add configs to site.config.ts, there are too many values, so what's your suggestion?
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.
@laymonage already remove the giscus repo information, made these keys into config for giscus.
(cherry picked from commit 559ba73)
This looks like a really great starting point for Giscus support, which I'm a fan of. I don't think it should be enabled by default or, as you mentioned, it should be optional and configured via the config file. |
Thanks for reply, I'll add config for Giscus ASAP. |
Is this not ready? @sgamerw @transitive-bullshit |
Glad to see this feature still works! |
…notion-starter-kit into transitive-bullshit-main # Conflicts: # package.json
@transitive-bullshit hi, already add configs and Giscus was disabled by default. |
already done,wait for reviewer to approve. |
Description
add giscus support, not add config to site.config.ts, maybe too simple and crude, but it works.
Notion Test Page ID
I test it locally, and comment success to giscus repo.
https://github.com/orgs/giscus/discussions/1160