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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a complete comment system for minima #702

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

YURLAK
Copy link

@YURLAK YURLAK commented Dec 29, 2022

Hello, developers!馃憢This is my first PR.

Problem.

Minima has less comment system.And disqus has been deprecated.

How to realize it?

I changed three places.

  • Added /includes/comments.html,Delete /includes/disqus_comments.html.
  • Added comment settings in _config.yml.
  • Changed comment part in _layouts/post.html.

At last

After my changes.Now Giscus,Utterance and Disqus are available now.
I tested it successfully on my blog.
And that's all.Thanks馃榾

@ashmaroli
Copy link
Member

ashmaroli commented Jan 1, 2023

Hello @YURLAK,
First of all, welcome to the world of Open Source Softwares.

Regarding your pull request, I love the idea but would like to leave some feedback nevertheless:

  • I would prefer not to reserve a top-level config key for this to avoid conflicts on existing sites. Therefore, I'd prefer {{ site.minima.comments.[NEW_KEY] }} instead of the current {{ site.[NEW_KEY] }}.
  • We no longer build the demo site using the master branch. So, adding the config details to _config.yml is not only redundant but additional information to delete for users forking the repository. I suggest you comment out all the comment configuration or move it to the README document.
  • For readability reasons, it's better that you wrap the attributes of the HTML script tags.
  • Lastly, I think you forgot to include the original Disqus code into the new include file.

@YURLAK
Copy link
Author

YURLAK commented Jan 3, 2023

Hello,@ashmaroli.Thanks for your feedback!According to your feedback,I made some changes.Readability does matter,I updated README.md that deleted some redundancy configuration.

Copy link

@louisroyer louisroyer left a comment

Choose a reason for hiding this comment

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

Some typographic suggestions.

README.md Outdated Show resolved Hide resolved

Optionally, if you have a Disqus account, you can tell Jekyll to use it to show a comments section below each post.
If you want to add comment system for your site,add the following code to the `README.md` file.If not,skip this part.

Choose a reason for hiding this comment

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

Suggested change
If you want to add comment system for your site,add the following code to the `README.md` file.If not,skip this part.
If you want to add comment system for your site, add the following code to the `README.md` file. If not, skip this part.

label: "Comments"
theme: "your_theme"

# You must install giscus github app before use.(https://github.com/apps/giscus)

Choose a reason for hiding this comment

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

Suggested change
# You must install giscus github app before use.(https://github.com/apps/giscus)
# You must install giscus github app before use (https://github.com/apps/giscus).

README.md Outdated Show resolved Hide resolved
@ashmaroli
Copy link
Member

ashmaroli commented Jan 3, 2023

@YURLAK I thought about this bit more. Continuing from previous feedback:

  • Comments are not handled by Jekyll itself so no point in having it as a top-level config key.
    Let's use {{ site.minima.comments_provider }} directly instead.
  • Rendering comments during development is not desired as it creates unnecessary discussions on Disqus. Perhaps, the same is true for Giscus and Utterances. Either ways, render comments only in "production".
  • The script rendered is easily accessible on provider's website. Therefore, we can avoid performance issues on larger sites by recommending users to copy from provider site (or our README) and hardcode user details as needed. We just provide a means to render desired provider script.
  • Additionally, provide a means to disable comments entirely for desired posts (via page.comments).
  • No built-in comment support to reduce coupling to third-party code.

So, our _layouts/post.html will have something like:

{% if jekyll.environment == "production" %}
  {% unless page.comments == "disabled" %}
    {% assign comments_provider == site.minima.comments_provider %}
    {% if comments_provider %}
      {% capture provider_filepath %}comments/{{ comments_provider }}.html{% endcapture %}
      {% include {{ provider_filepath }} %}
    {% endif %}
  {% endunless %}
{% endif %}

Then, we document what users need to do, in our README along with the note that build will fail if user does not provide associated include file.

YURLAK and others added 2 commits January 9, 2023 09:41
Co-authored-by: Louis Royer <55180044+louisroyer@users.noreply.github.com>
Co-authored-by: Louis Royer <55180044+louisroyer@users.noreply.github.com>
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.

None yet

3 participants