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.

Sign up for GitHub

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

Jump to bottom

Enable comments and reactions on blog posts #41

Closed
wants to merge 2 commits into from
Closed

Enable comments and reactions on blog posts #41

wants to merge 2 commits into from

Conversation

anchalshivank
Copy link
Contributor

No description provided.

@anchalshivank anchalshivank force-pushed the feature/enable-reactions-and-comments branch from 896ae49 to eed90ec Compare September 12, 2024 17:32
sergefdrv
sergefdrv requested changes Sep 13, 2024
View reviewed changes
Copy link
Member

@sergefdrv sergefdrv left a comment

Choose a reason for hiding this comment

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

We need to solve the issue with useBlogPost. And please remove the merge commit and rebase instead. As a rule of thumb, you should hardly ever have merge commits in your PRs.

src/theme/BlogPostItem/index.js Outdated Show resolved Hide resolved
blog/2024-03-04-on-implementation-of-distributed-prtocols/index.mdx Outdated Show resolved Hide resolved
@sergefdrv sergefdrv reopened this Sep 13, 2024
@sergefdrv
Copy link
Member

Please continue working in this PR. Just force-push into it.

@anchalshivank anchalshivank force-pushed the feature/enable-reactions-and-comments branch from ecab735 to 4948425 Compare September 13, 2024 11:22
@anchalshivank anchalshivank force-pushed the feature/enable-reactions-and-comments branch from 4948425 to 58927fa Compare September 13, 2024 11:27
sergefdrv
sergefdrv requested changes Sep 13, 2024
View reviewed changes
Copy link
Member

@sergefdrv sergefdrv left a comment

Choose a reason for hiding this comment

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

Please don't add draft field to the front matter of the existing posts. Also, revert other unintended changes. (It's always good to have a look at the changes before requesting review.)

src/theme/BlogPostItem/index.js Outdated Show resolved Hide resolved
src/theme/BlogPostItem/index.js Outdated Show resolved Hide resolved
src/theme/BlogPostItem/index.js Outdated Show resolved Hide resolved
src/theme/BlogPostItem/index.js Outdated Show resolved Hide resolved
@sergefdrv
Copy link
Member

sergefdrv commented Sep 13, 2024 •
edited
Loading

There's also a problem when switching between dark and light themes, but Disqus renders correctly after reloading the page. I guess we should just tolerate this.

@anchalshivank
Copy link
Contributor Author

There's also a problem when switching between dark and light themes, but Disqus renders correctly after reloading the page. I guess we should just tolerate this.

I think we can use useEffect to solve this issue

sergefdrv
sergefdrv requested changes Sep 16, 2024
View reviewed changes
Copy link
Member

@sergefdrv sergefdrv left a comment

Choose a reason for hiding this comment

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

Great that you found a solution for the issue with switching dark/light mode!

Please try to always check what commits you push and have a look what the changes look like. Now there are two commits in this PR that should be squashed together.

docusaurus.config.js Outdated Show resolved Hide resolved
docusaurus.config.js Outdated Show resolved Hide resolved
src/theme/BlogPostItem/index.js Outdated Show resolved Hide resolved
src/theme/BlogPostItem/index.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@anchalshivank anchalshivank force-pushed the feature/enable-reactions-and-comments branch from 2495995 to 9f9f5f4 Compare September 16, 2024 16:52
@sergefdrv sergefdrv changed the title Enable comments and reactions on the blog post Enable comments and reactions on blog posts Sep 17, 2024
sergefdrv
sergefdrv requested changes Sep 17, 2024
View reviewed changes
package.json Outdated Show resolved Hide resolved
src/theme/BlogPostItem/index.js Outdated Show resolved Hide resolved
src/theme/BlogPostItem/index.js Outdated Show resolved Hide resolved
src/theme/Footer/Copyright/index.js Show resolved Hide resolved
@anchalshivank anchalshivank force-pushed the feature/enable-reactions-and-comments branch from 5c5cbf4 to f123f98 Compare September 21, 2024 15:02
sergefdrv
sergefdrv requested changes Sep 23, 2024
View reviewed changes
Copy link
Member

@sergefdrv sergefdrv left a comment

Choose a reason for hiding this comment

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

Please remove the merge commit and rebase instead. As a rule of thumb, you should hardly ever have merge commits in your PRs.

src/theme/Footer/Copyright/index.js
@@ -6,7 +6,7 @@ export default function CopyrightWrapper(props) {
const { siteConfig: { customFields: {
siteLicense,
} } } = useDocusaurusContext();

Copy link
Member

Choose a reason for hiding this comment

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

Now there is a trailing whitespace...

src/theme/BlogPostItem/index.js
const { metadata } = useBlogPost();
const { frontMatter, permalink, title } = metadata;
const currentUrlOrigin = typeof window !== 'undefined' ? window.location.origin : '';
const fullPermalink = `${currentUrlOrigin}${permalink}`;
Copy link
Member

Choose a reason for hiding this comment

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

I believe we should use ${siteConfig.url} instead of ${currentUrlOrigin} here (see this).

src/theme/BlogPostItem/index.js
colorMode={disqusColorMode}
shortname={shortname}
config={{
url: fullPermalink,
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I thought metadata.permalink is the full URL. Now I see it doesn't include the base URL. Then I think we can use it as the discussion identifier and add identifier: permalink.

src/theme/BlogPostItem/index.js
{comments && (
<DiscussionEmbed
// This property is not used by `DiscussionEmbed`, but forces React to re-render it upon theme changes
colorMode={disqusColorMode}
Copy link
Member

Choose a reason for hiding this comment

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

React complains about this property's name:

React does not recognize the `colorMode` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `colormode` instead.

Let's make it happy by using colormode instead.

@sergefdrv sergefdrv linked an issue Oct 29, 2024 that may be closed by this pull request
Enable reactions and comments to blog posts #7
Open
@sergefdrv
Copy link
Member

Blog post pages now throw "Uncaught runtime errors" in development mode:

ERROR
Cannot destructure property 'address' of '(intermediate value)' as it is undefined.
    at ConnectorClass.onMessage (chrome-extension://fldfpgipfncgndfolcbkdeeknbbbnhcc/extensionPageScript.js:1584:13)
    at handleMessage (chrome-extension://fldfpgipfncgndfolcbkdeeknbbbnhcc/extensionPageScript.js:1607:15)

The errors disappear when removing Disqus' DiscussionEmbed. There must be something wrong with it... Perhaps, we should not integrate with Disqus.

@sergefdrv sergefdrv closed this Oct 29, 2024
@sergefdrv sergefdrv removed a link to an issue Oct 29, 2024
Enable reactions and comments to blog posts #7
Open
@sergefdrv sergefdrv mentioned this pull request Oct 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sergefdrv sergefdrv sergefdrv requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@anchalshivank @sergefdrv