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

[Optional feature]: Staticman Nested Comment Support #119

Closed
wants to merge 28 commits into from

Conversation

VincentTam
Copy link
Contributor

@VincentTam VincentTam commented Dec 19, 2018

Motivation

Provide a free commenting service.

Hugo's built-in Disqus has no Markdown support. Staticman is open-source and transmits HTML form data as YML files to be merged in GitHub/GitLab repos. Despite its MIT license, the project had been bundled with GitHub for years. Thanks to Nicholas Tsim's PR, Staticman now supports GitLab.

To add Staticman, I've combined Minimal Mistakes and Beautiful Hugo's code so as to avoid breaking the original modals. I am not good at CSS at the moment, so the visual layout will be strange. It took me hours to understand the role of container. During local preview, the SASS comes out. However, when it's deployed to GitLab, this has disappeared. However, the logic is already there at https://vincenttam.gitlab.io/introduction. Web developers should be able to fix the display much more efficiently than I do. I'm creating this PR to publish my code.

screenshot_2018-12-18-17-48-55

Features

  • Staticman comments
  • Reply function
  • sample config files (site, repo, GitLab config) in exampleSite
  • CSS
  • Documentation

Quick guide to test this PR

I assume the user has installed his/her Hugo theme(s) as a Git submodule.

$ git remote -v
upstream	https://github.com/vickylai/hugo-theme-introduction.git (fetch)
upstream	https://github.com/vickylai/hugo-theme-introduction.git (push)
... # other remote omitted
$ git checkout -b pr119 # test on a new branch pr119
$ git fetch upstream pull/119/head # git pull will be rejected
$ git merge FETCH_HEAD # manually merge the this PR against branch pr119
$ cd <your-blog>
... # edit your .gitmodules with url="<repo-containing-pr119>" and branch = "pr119"
$ git submodule sync # inform Git the changes in .gitmodules
$ git submodule update --remote --recursive # switch to the HEAD of your cloned repo for the theme

On the recent Staticman's public API bottleneck

Due to eduardoboucas/staticman#227, the parameter api in config.toml defaults to the API address associated with @staticmanlab.

Missed "container"
i18n doesn't work under assets.  I've adopted Minimal Mistakes'
approach to get this work.
I've done d818b0c against the wrong branch!
1. Forgotten the last "fieldset" originally present in Bruno Adele's
code for Beautiful Hugo's native Staticman support
2. Clear form after successful submission
3. Shorter jQuery code to clear all form (and hidden) input fields
@hanzei hanzei self-requested a review December 19, 2018 13:42
@VincentTam
Copy link
Contributor Author

VincentTam commented Dec 19, 2018

Look back at what I've written, I believe that a self-review would help others.

  • unfinished SASS: perhaps the instances of px has to be changed. The original files can be found by searching "comments" among Minimal Minstake's SCSS files. I've abandonned Bruno Adele's CSS file since the code for modal clashes with those for "projects" in the hompage.

  • Staticman nested comments: the Go-HTML code is copied and adapted from Beautiful Hugo PR 222, so most points in the "remarks & rooms for improvement" in [Feature] Staticman Comment Reply halogenica/beautifulhugo#222 (comment) applies to this PR as well, except

    • navbar in this theme doesn't float, so the point "I've introduced an offset" is irrelevant in this PR.
    • Internationization of "reply to" in the link button is missing introduced as comment_btn_submit_reply at 1be3757. I can't find a have to get Go-HTML template code parsed in assets/js/staticman.js. To speed up development, I've adopted Minimal Mistake's approach and put the JS code inside a <script> tag in an HTML file.
    • I don't wish to change the <article id="comment-1"> to <article id="comment-{{ ._id }}"> in layouts/partials/staticman.html, provided that this repo has no previous Staticman support. That simplifies things, and renders the following function redundant in layouts/partials/js/staticman.html. However, I would be like to a take break from doing the logic.
    // smooth scroll to different link anchor
    $('.comment-reply-target a[href^="#"]').click(function (){
    targetPostID = $(this).attr('href');
    targetID = "#" + $(targetPostID).parents('.js-comment').attr('id');
    $('html, body').animate({ scrollTop: $(targetID).offset().top }, 500);
    });

    P.S. If such change of id attribute were introduced in Beautiful Hugo, all URL to existing static comments on web sites powered by Beautiful Hugo would be broken unless the site owner/maintainer conducts a URL redirection scheme---that's too complicated for personal blog owners. Breaking URL has bad impacts on UX and Google Bot search, so that's bad for the site's SEO. As a result, I had to kept Bruno Adele's code for the id attribute and develop this JS workaround. However, in repos which have no existing Staticman support (like this one), there's no need to make such considerations.

I would appreciate any thoughts and code changes.

@hanzei
Copy link
Collaborator

hanzei commented Jan 10, 2019

@VincentTam First of all: Thank you very much for the time and effort you put into this! The code you wrote it very elegant and fits well into hugo.

But I'm not sure if this is something I want to add to this theme. The theme is about setting up a simple and minimalistic personal site. Blogging is just a site feature of this theme. I assume most people just want to set up there site and don't care about comments.

@VincentTam Thoughts on this? Out of interests: Why did you put work into this theme and not any other?

@vickylai Would love to hear your thoughts on this!

@VincentTam
Copy link
Contributor Author

Thank you for your reply despite my unfinished work. Since I'm not a developer, I publish it here for interested users.

This PR aims at providing comment support alternative to existing Disqus support. For Disqus vs Staticman, you may google for the rationale of preferring the later. (I'm replying on mobile.)

Whether comment is important is subjective. A theme designer may (or may not) offer the freedom for its users to enable this feature. With Staticman's config parameters missing or blank (default), such features will be disabled.

Looking back, my code isn't the best. I've messed up the code for the reply target in each comment reply. Please see the last point for details.

I did integrate Staticman into other templates. You may consult

  1. GitLab Pages/Hugo MR ?
  2. GitLab Pages/Jekyll MR ?
  3. https://framagit.org/staticman-gitlab-pages

In fact, this is a derivative of halogenica/beautifulhugo#222, which aims to fix the logic problem mentioned in dancwilliams/networkhobo#373.


IMHO, interactions with readers are beneficial to site owners, who often get great ideas from readers. Moreover, having static comments (with Gravatar of other users) is great in terms of SEO because search engines view this as the activities of the page.

@victoriadrake
Copy link
Owner

But I'm not sure if this is something I want to add to this theme. The theme is about setting up a simple and minimalistic personal site. Blogging is just a site feature of this theme. I assume most people just want to set up there site and don't care about comments.

@vickylai Would love to hear your thoughts on this!

I’d leave this one to the community to decide, if anyone wishes to weigh in. Introduction is meant to be a minimalist personal site, but I’m not opposed to the addition of comment functionality, as long as it is clearly optional. Especially with Staticman over Disqus.

I would like to be able to take a look at the styling within the next couple weeks. If anyone else wants to give it a go in the meantime, please do!

@VincentTam
Copy link
Contributor Author

VincentTam commented May 21, 2019

I've recently revised the template code in another Hugo theme Huginn. The reasons are two-folded.

  1. Unnecessary JS code for smooth-scrolling and complicated Go-HTML template code

    That was unintentionally copied from the original PR for Beautiful Hugo, which is the theme that I'm now using and tweaking, and which has existing Staticman integration code that I don't wish to break.

  2. Mixed JS code and Go template code for internationalization.

    I copied this from the Jekyll theme Minimal Mistakes. A more unobstrusive approach would be to load site data into Go-HTML template files (with some hidden elements), and operated on them using JavaScripts in the assets, instead of injecting the locale data strings into the JS code.

Since the Go template & JS code used for both themes were essentially the same at the beginning, an overall comparison git diff {start}..{end commit} on Git repo for that theme would get rid of dirty stuff in the middle of the commit history. I'll leave that for interested users.

@victoriadrake
Copy link
Owner

Hello @VincentTam! I made some updates to your work but am unable to push to this PR. If you'd like to keep it going, would you kindly follow these instructions to grant me permission to make edits?

If you don't want to do that, you can still look over my changes in the new branch and see if there's anything you'd like to implement on your own.

@VincentTam
Copy link
Contributor Author

@victoriadotdev Thanks for merging staticman3 and your changes. Regarding the permissions problem, that's unexpected. I usually follow the defaults for PR submission and leave the checkbox "allow changes from maintainers" checked. As a result, that's what I can see from my qutebrowser on Xubuntu 18.04.

Screenshot_2019-07-22_08-02-08

At the bottom right corner of the screenshot, I can see that the relevant checkbox is checked. I'm going to contact GitHub support for that.

@VincentTam
Copy link
Contributor Author

@victoriadotdev I've just got a reply from GitHub, who suggested me to uncheck and check the checkbox. I've done that, and please verify whether the write permission problem has been solved.

@VincentTam VincentTam changed the title [Optional feature]: Unfinished Staticman Nested Comment Support [Optional feature]: Staticman Nested Comment Support Aug 2, 2019
@VincentTam
Copy link
Contributor Author

VincentTam commented Aug 3, 2019

Hello @VincentTam! I made some updates to your work but am unable to push to this PR. If you'd like to keep it going, would you kindly follow these instructions to grant me permission to make edits?

@victoriadrake Another update: Despite further discussions with GitHub Support staff, she couldn't find out the reason why you were unable to push to this PR. Since this PR's submission, the write permissions from maintainers have always been enabled. I tried asking if your recent change of user name had contributed to this error, by she was unable to verify this theory after some tests.

It seems that this push failure is due to past conflicts of translation files, in which your user name appeared at the bottom.

  1. I tried to port a section of UI text from Minimal Mistakes and Beautiful Hugo 6 months agos.
  2. You've recently changed your user name, which appeared at the bottom of the file.

As a result, each translation file is modified on both sides at the end of the file, and this causes a merge conflict. As can be seen from the commit tree of the HEAD of this PR, I've merged your changes this week, so the conflicts have been resolved. Hope this solves your difficulties in committing against the head of this PR.


A self-review on the recent changes of this PR

  1. Replaced Beautiful Hugo's logic for looping over .Site.Data.comments with my own one: the former sets the id attribute of each static comment as comment-N. I've changed that to the _id field of the YML data file due to this scenario: in case of moderation:false, comments are published once submitted. Site owners might want to delete a few unwanted ones afterwards. After such deletion, the URL of every static comment after the deleted comment would change. Existing links pointing to those comments would not function as desired. Those such possibility is marginal for minimalistic and personal static sites, but this issue of URL shifting can be avoided by using the _id field unique to each static comment.

    As a consequence, I've removed the JS code for smooth scroll in the Staticman script.

  2. Moved Staticman JS script from a <script> tag under layouts/partials/js to the assets folder. Now, the role of layouts/partials/js/staticman.html is to

    • include a <script> tag pointing to assets/js/staticman.js
    • excecute the Hugo code inside assets/js/staticman.js for constructing <form action> URL with JavaScript due to the behavior of some search bots described in this blog post. An example of form action URL in JavaScript: this blog post.

    Introduction + Staticman demo site new UI

  3. Improved JS code for setting reply target when the reply button is clicked.

    <a class='reply-btn' href='#comment-form' title='{{ ._id }}'>{{ i18n "replyToMsg" }}</a>

    <a class='reply-btn' href='#comment-form' title='{{ .replyThread }}'>{{ i18n "replyToMsg" }}</a>

    The title='{{ ... }}' here is crucial in the construction of internal referencing of nested comments. That would lead to an unhelpful tooltip text showing the _id of the static comment's eldest ancestor.

    • A CSS rule can be added to hide this if you want; or
    • store this piece of info elsewhere (say, in the form of a hidden <span> tag) under the <article> tag for the static comment.
  4. Added a functionality for displaying replying target name and avatar on top of the comment form when the "reply to this comment" button is clicked. Clicking a ❌ button on the right of the reply target display would remove the reply target. Clicking on another reply button under another comment would update the reply target display.

  5. Improved display and introduced three SASS variables in Staticman SASS file for calculating the appropriate width of a comment reply.

    width: calc(100% - #{$avatar__size} - #{$avatar__sep} - 1px)

    The variables can be adjusted. Currently, the avatar is so large that this doesn't work well on mobile devices.

As before, please feel free to change anything and/or leave a comment/question(s).

Copy link
Contributor Author

@VincentTam VincentTam left a comment

Choose a reason for hiding this comment

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

Practice GitHub review.

$('.page__comments .comment').on('click', '.reply-btn', function (evt){
resetReplyTarget();
var cmt = $(evt.delegateTarget);
$('.page__comments input[name="fields[replyThread]"]').val(this.title);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you find my approach of storing the _id of a comment's eldest ancestor in the title attribute of an <a> tag not satisfying, I can change the this.title to replyThread, where var replyThread = cmt.find('.someClassName').

1. Add Staticman bot/app, depending on your Git service provider.
- GitHub: choose either one of the following method
+ GitHub App: refer to issue
[https://github.com/eduardoboucas/staticman/issues/243](https://github.com/eduardoboucas/staticman/issues/243)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@staticmanapp and https://api.staticman.net/v2 no logner work, as reported in eduardoboucas/staticman#306.

Then help **[@staticmanlab](https://github.com/staticmanlab)**
accept the invitation by going to

https://staticman3.herokuapp.com/v3/connect/github/<username>/<repo-name>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Staticman's documentation only covers v2, so there's a need to give a self-contained setup guide.


$.ajax({
type: $(this).attr('method'),
url: [endpoint, 'v3/entry', gitProvider, repo, branch, 'comments'].join('/'),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separated POST request URL in JS code to prevent abuses by spam bots through scraping the generated HTML source code.

/* Static comments */
$avatar__size: 5rem
$avatar__sep: 3rem
$reply__indent: 2rem
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel free to change the values here. Since I'm not a web designer, I haven't considered the mobile loading issue.

Error made while incorporating code from two different themes:
1. Beautifhul Hugo
2. Minimal Mistakes
@github-actions
Copy link

github-actions bot commented Mar 1, 2020

It looks like this PR has been idle a while, so I am marking it as stale. Remove the label or comment if this is still being worked on.

@hanzei hanzei requested review from victoriadrake and removed request for hanzei March 4, 2020 08:44
@github-actions github-actions bot closed this Mar 10, 2020
@hanzei hanzei reopened this Mar 10, 2020
@github-actions
Copy link

It looks like this PR has been idle a while, so I am marking it as stale. Remove the label or comment if this is still being worked on.

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

Successfully merging this pull request may close these issues.

None yet

3 participants