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

[Feature] Staticman Comment Reply #222

Open
wants to merge 31 commits into
base: master
from
Open

Conversation

@VincentTam
Copy link
Contributor

@VincentTam VincentTam commented Nov 16, 2018

Demo site

https://vincenttam.gitlab.io/bhdemo

screenshot_2018-11-16 my second post 2

New features

  • comment reply
  • comment preview

Bug fixes and modifications

Backward compatibility

I tested this for another existing Beautiful Hugo site. Despite some missing fields (c.f. staticman.yml) in existing YML comment data, this works fine.

old sites using this PR

Quick guide to test this pull request

If you're using a theme as a Git submodule (as recommended in Hugo's official tutorial), you may create a new (local/GitLab/etc) (private) repo for this theme and run these commands to get this PR merged against a testing branch pr222 in the private repo. I denote the URL of this repo as the upstream.

$ git remote -v
upstream	https://github.com/halogenica/beautifulhugo.git (fetch)
upstream	https://github.com/halogenica/beautifulhugo.git (push)
... # other remote omitted
$ git checkout -b pr222 # test on a new branch pr222
$ git fetch upstream pull/222/head # git pull will be rejected
$ git merge FETCH_HEAD # manually merge the this PR against branch pr222
$ cd <your-blog>
... # edit your .gitmodules with url="<repo-containing-pr222>" and branch = "pr222"
$ 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

Remarks rooms for improvement

I'm not a developer. I wrote this based on my basic knowledge in web markup and scripting languages, @dancwilliams's great example, some Google search results and trial-and-error. This commit history is far from clean: I've changed some important field names and file structure a few times.

  • I don't wish to change the <article id="comment-1"> in layouts/partials/staticman-comments.html, so I store the comment _id in a "reply to XYZ" link button.
  • Reply comments have id="comment-1r1". Though I call them "nested comments", a reply comment is not an HTML children() of any .static-comment.
  • I've changed {{ if not .parent }} in layouts/partials/staticman-comments.html (introduced at d68cf2e by @badele) to {{ if not .replyThread }} because options[parent] is for subscription entries. It's clear (to me, at least) that @badele intends to loop over all "main comment" (as in eduardoboucas/staticman#35) first.
    • I've chosen replyThread instead of threadID because a "main comment" starts a thread, so it belongs to a thread, thus it would look a bit strange without a threadID.
    • For each reply comment, there's (1) main comment, and (2) reply target, which aren't identical in general. Provided the double-loop structure discussed in eduardoboucas/staticman#35 and implemented at d68cf2e, whose outer loop iterates through (1) main comment and inner loop iterates through the reply recomment, there's two possible ways to reconstruct the whole reply tree, and this PR inclines to the Staticman side more than the Hugo side.
  • CSS can be improved, but the logic is already there.
  • I've introduced an offset in static/js/staticman.js so that the anchor target won't get covered by the navbar. Maybe there's a pure CSS solution.
  • I kept @dancwilliams's choice of using <a> instead of <button> for reply button. After hours of trial-and-error, I've to introduce either a delay or an animation (the way I prefered) so that I can jump to a different element (the top of the post) by feeding another href (id of the "reply to" button)
  • Internationization of "reply to" in the link button is missing.

See also

@VincentTam VincentTam changed the title Add reply functionality [Feature] Staticman Comment Reply Nov 20, 2018
VincentTam added 2 commits Nov 20, 2018
Changed UI text from "Reply to {{ .user }}" to "Reply to this message"
to simplify template code.  In western European languages, the
original UI text reads "{{ i18n replyToMsg }} {{ .user }}", but this
order is reversed in some languages (e.g. German).

Languages that I know: Chinese, English, French

Sources for other translations:

- Norwegian Bokmål `nb`:

    Tenker du på deg selv som en bruker nå som du er på internett, eller noe helt annet?

    https://nrkbeta.no/2018/10/18/tenker-du-pa-deg-selv-som-en-bruker-na-som-du-er-pa-internett-eller-noe-helt-annet/

    Scroll to bottom to the comments section to find the f

- Japanese `ja`: WordPress コメント機能の基本的な使い方 – 承認・返信・拒否・削除

    https://ring-of-life-netbiz.com/wordpress-comment

    https://i2.wp.com/ring-of-life-netbiz.com/wp-content/uploads/2016/12/wordpress-comment6.png?resize=591%2C386&ssl=1

- The translation for all other languages are copied from the key
  "$ReplyToThisComment" in
  shikhadongre/chamilo.classic@e2c90e9
Loading of renderMathInElement(document.body) in jQuery caused error,
blocking the load of custom KaTeX macros in per site /static/js/.
With this line removed, the custom KaTeX macros have managed to run
again.
VincentTam added 2 commits Dec 12, 2018
@VincentTam
Copy link
Contributor Author

@VincentTam VincentTam commented Jul 29, 2019

Update: Towards a more static nested comments support

Before fe547af, the anchor in each comment reply is too complicated. It finds the id attribute of the reply button in the reply target (stored as targetPostID) from href in the anchor a.comment-reply-target. Then targetPostID is used to find the true target id in the form #comment-xry. Finally, the behaviour of the onRelease event of the anchor is tweaked with animate in the JavaScript so that it would smoothly scroll to the true target.

$('.comment-reply-target a[href^="#"]').click(function (){
targetPostID = $(this).attr('href');
targetID = "#" + $(targetPostID).parents('.static-comment').attr('id');
$('html, body').animate({ scrollTop: $(targetID).offset().top-$('nav').height() }, 500);
});

Get the whole picture from two separate files are hard, so as the difficulty to maintaint them. The recent commits attempt to directly compute the right anchor links by Hugo (as a staticman site generaor). i.e. The anchor a.comment-reply-target now gives href like comment-1r1, so that there's no need to use the reply button as an intermediate.

The new way makes use of Hugo's .Scratch with maps. Each post ID is used as a mey to maps to an integer representing the order of the post in a thread. Each main post (those without parents) are assigned 0. The first comment reply in a thread is assigned 1, and so on... The line {{ $.Scratch.Delete "replyIndices" }} is optional. This PR works fine without resetting the Scratch mapping because the ID for posts are unique.

diff --git a/layouts/partials/staticman-comments.html b/layouts/partials/staticman-comments.html
index e3c21dd..9d8a0a4 100644
--- a/layouts/partials/staticman-comments.html
+++ b/layouts/partials/staticman-comments.html
@@ -20,6 +20,7 @@
         {{ $.Scratch.Add "hasComments" 1 }}
         {{ $.Scratch.Set "hasReplies" 0 }}
         {{ $.Scratch.Set "threadID" ._id }}
+        {{ $.Scratch.SetInMap "replyIndices" ._id 0 }}
         <article id="comment-{{ $.Scratch.Get "hasComments" }}" class="static-comment">
           <img class="comment-avatar" src="https://www.gravatar.com/avatar/{{ .email }}?s=48">
           {{ if .website }}
@@ -37,6 +38,7 @@
         {{ range $comments }}
           {{ if eq .replyThread ($.Scratch.Get "threadID") }}
             {{ $.Scratch.Add "hasReplies" 1 }}
+            {{ $.Scratch.SetInMap "replyIndices" ._id ($.Scratch.Get "hasReplies") }}
             <article id="comment-{{ $.Scratch.Get "hasComments" }}r{{ $.Scratch.Get "hasReplies" }}" class="static-comment static-comment-reply">
               <img class="comment-avatar" src="https://www.gravatar.com/avatar/{{ .email }}?s=48">
               {{ if .website }}
@@ -44,7 +46,9 @@
               {{ else }}
               <h4 class="comment-author">{{ .name }}</h4>
               {{ end }}
-              <h5 class="comment-reply-target"><a href="#{{ .replyID }}"><i class="fas fa-reply"></i> {{ .replyName }}</a></h5>
+              {{- $replyTargetIndex := (index ($.Scratch.Get "replyIndices") .replyID) -}}
+              {{- $replyLinkEnd := cond (eq $replyTargetIndex 0) "" (print "r" $replyTargetIndex) -}}
+              <h5 class="comment-reply-target"><a href="#comment-{{ $.Scratch.Get "hasComments" }}{{ $replyLinkEnd }}" title="{{ .replyID }}"><i class="fas fa-reply"></i> {{ .replyName }}</a></h5>
               <div class="comment-timestamp"><a href="#comment-{{ $.Scratch.Get "hasComments" }}r{{ $.Scratch.Get "hasReplies" }}" title="Permalink to this comment"><time datetime="{{ .date }}">{{ dateFormat (default (i18n "shortdateFormat") .Site.Params.dateformat) .date }}</time></a></div>
               <div class="comment-content"><p>{{ .comment | markdownify }}</p></div>
               <div class="comment-reply-btn">
@@ -53,6 +57,7 @@
             </article>
           {{ end }}
         {{ end }}
+        {{ $.Scratch.Delete "replyIndices" }}
       {{ end }}
     {{ end }}
   {{ end }}

Removed JavaScript smooth scroll to keep things simple.

diff --git a/static/js/staticman.js b/static/js/staticman.js
index b874723..1e6b027 100644
--- a/static/js/staticman.js
+++ b/static/js/staticman.js
@@ -52,11 +52,4 @@
     $('input[name="fields[replyName]"]').val("");
     $('.js-form fieldset button.button').text('Submit');
   });
-
-
-  $('.comment-reply-target a[href^="#"]').click(function (){
-    targetPostID = $(this).attr('href');
-    targetID = "#" + $(targetPostID).parents('.static-comment').attr('id');
-    $('html, body').animate({ scrollTop: $(targetID).offset().top-$('nav').height() }, 500);
-  });
 })(jQuery);
VincentTam added 7 commits Jul 29, 2019
Accidentally removed in previous merge with upstream.  Needed for reply button.
1. Comment author, reply target author and time are at the same line.
2. Improved comment button appearance.
3. Show reply target name and avatar on top of the comment form.
4. Copied notice about email addr & req'd field from Minimal Mistakes.
5. Replaced modal with notice under the comment form like MM.
6. Disable the comment form during form submission like MM.
7. Clear comment preview after clicking "reset".
Careless mistake during diff patch
1. More visible close button in reply target display
2. Unified button text color to solarized $base03
3. Enlarged reply target avatar by 1rem
4. Removed "small" class in MM's commentFormInfo for consistency
@VincentTam
Copy link
Contributor Author

@VincentTam VincentTam commented Jul 30, 2019

Improved UI with really static links.

Screenshot_2019-07-30 Huginn Theme With Staticman(1)

Confused code with other themes.  The comment id "comment-XrY" human-readable,
while the replyID field value is generated.
@halogenica
Copy link
Owner

@halogenica halogenica commented Sep 21, 2019

Hey @VincentTam, Thank you so much for all your hard work! I'd love to merge this. Github is complaining about an unresolvable conflict, and I think it's just upset that you moved the .css and .js from static/* to assets/*. Can you tell me why they moved? Thank you!

@VincentTam
Copy link
Contributor Author

@VincentTam VincentTam commented Sep 21, 2019

The motivation for such refactoring is the same as daattali/beautiful-jekyll@19c9d06: to avoid spam comments by moving the form action URL to JS, so that a static HTML scan performed by search bots won't find the URL. To do that, we need some Hugo code in the asset JS file to read the custom Staticman API from the site config file.

Issues to be fixed:

  1. Display issue
    Example: https://vincenttam.gitlab.io/post/2018-09-27-custom-katex-macros/
    Screenshot from 2019-09-21 09-30-55
  2. Comments preview
    I've used Showdown for comments preview. However, it's not the same flavor of Hugo's version.
    Here's working Jekyll example of Markdown preview: https://zcrc.me/retromini/2019/05/31/opendingux-release-20190601.html#replies
@iootaa
Copy link

@iootaa iootaa commented Jun 26, 2020

Hi all, and thank you @VincentTam to work on this feature

The demo you refer to is not working anymore : https://vincenttam.gitlab.io/post/2018-09-27-custom-katex-macros/

Cheers

@sirinath
Copy link

@sirinath sirinath commented Jul 6, 2020

Can we have the ability to:

  • have up and down votes on comments and replies
  • star or rate comments and replies
  • collapse and expand comments and threads
  • add reaction using emoji icons to comments
  • display a discourse like summary of the comment thread at the top
  • display a colour-coded line to the left grouping reply threads by level
  • ability to sort thread by the user on:
    • time updating thread
    • votes
    • rating
    • reactions
  • toddle threaded and linearised format
@VincentTam
Copy link
Contributor Author

@VincentTam VincentTam commented Jul 24, 2020

@sirinath yes but git is not made for huge data storage like SQL. stars accompanied by a written comment can be treated as numbered field, but I worry that FB-like likes/thumb ups would generate too much interactions, which are then transformed into an illusory "code source change", that your Git hosting site isn't made for. An SQL database is a more sensible solution.

looking back, i've put too much things into one PR. i shld hav broken it into several, but i dun hav the time to do so. my tweaked theme on GitLab at https://gitlab.com/VincentTam/beautifulhugo works well for my blog https://vincenttam.gitlab.io, so i won't bother to maintain it. as can be seen from users in the official Staticman repo, there's a strong demand for a working Staticman example. i believe leaving it untouched here might help other courageous testers, especially the sample config files staticman.yml and config.toml.

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

Successfully merging this pull request may close these issues.

None yet

4 participants