-
Notifications
You must be signed in to change notification settings - Fork 194
Staticman nested comments support from Huginn #69
Conversation
which is in turn a port from halogenical/beautifulhugo#222. Introduced true nested comments with reply functionality. 1. Reply target anchors to easily jump between replies. 2. Interactive reply target display with Gravatar and name. 3. Improved input type for "website" field. 4. Implemented FR pacollins#63 to construct POST URL in JS. 5. Clearer instructions for Google reCAPTCHA v2. 6. Introduced some SCSS variables for layout control 7. Fixed missing "id" attribute in each comment. 8. Fixed missing translation UI text for Staticman forms. 9. Changed default API endpoint to the one serving @staticmanlab due to eduardoboucas/staticman#307.
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.
There are accessibility issues in these changes.
Forgotten to update master branch before branching
Gravatar alt text wasn't there in the original code, but I agree with the bot.
Marginally related: I have removed the Accesslint App for the time being. I may reintroduce it once things settle and we can focus on accessibility. |
@pacollins Please review my PR. I prefer porting from my existing nested comments templatesest rather than re-inventing the wheel. Here's some (possible) bugs in the current Staticman template. For background and motivations about nested comments, please see https://github.com/onweru/hugo-swift-theme/projects. Test site short link: https://lstu.fr/fish-demo, https://vincenttam.frama.io/fish-demo hugo-future-imperfect-slim/layouts/post/staticman.html Lines 2 to 4 in afbf3cc
Even though this PR is not as important as #47 and #71, the possible spamming problem mentioned in #63 can be quite annoying. For example, you may see https://github.com/hashtafak/hashtafak.github.io/commits/master. I confess that I didn't know Node.JS when I created my Staticman instance for testing. I've tried ways like reCAPTCHA v2 and I've overlooked issue eduardoboucas/staticman#307 when I proposed to use the official production Staticman instance at b75aa75#diff-b7cd599ec3be3f913bdf453cbd849f0d.
Use
hugo-future-imperfect-slim/layouts/post/staticman.html Lines 33 to 35 in afbf3cc
|
I will review now. I needed time to properly test (which I have at this moment). |
As of right now, using @staticmanlab:
Form HTML is from the localhost:
|
Perhaps some images like might help understanding my code. To give a rough idea about the entire script, there're three parts in the Staticman script.
|
Not gonna lie, Staticman goes over my ability. I still can't get a comment to post. Receiving the following from the console:
and
|
@pacollins To correctly test Staticmn integration for this blog theme,
|
Fixed
We need to move EDIT: EDIT: See comment below. |
I don't get this. It's clearly said that in the docs that this file has to be root-level. For a self-contained setup guide, I've recently created a Wiki page for Staticman setup in Hugo Swift Theme. I'm sorry that my hands type quite slow. |
Well, I take my statement back and clarify it. We need to add documenation somewhere that makes it clear you must put your entire Hugo folder (pre-built and That all being said, it would cause more issues for them (having to individually download comments), so we (as a theme or as staticman) should probably make sure it is clear you need to push your whole pre-built site to the server and not just public when using staticman. Does that edge-case thought process make sense? You did nothing wrong, I just represented someone who wasn't thinking and could have misinterpreted the instructions. That all being said, I can now happily test your changes. 😄 |
I wonder what your deploy workflow is. My demo site (see my 2nd comment for the links) is using GitLab CI, which gives a publicly accessible file tree of Note the difference between "root of the site" and "root of the repository". In the Staticman setup, it shows the later. The Staticman repo config file In eduardoboucas/staticman#264 (comment), @eduardobucas wrote
Staticman is not related to the site. It communicates with the GitHub/GitLab repo. For a barebone example, you may see https://gitlab.com/ntsim/test-staticman/ (requires GitLab account). Another theoretically (but not practically) useful example at https://github.com/sdessus/comments_blog_tdm illustrates the functioning of Staticman (as a Node.JS app), even though what the owners claimed in their blog is apparently incorrect.
This repo is not practically useful since the comments for all posts have been mixed. The above claim is wrong since JS's
I prefer using the word "source" over "pre-built". It has to be, for the moment and near future, on the source branch in a ( custom instance of) GitHub/GitLab repo, since only these two types of remote Git repo are supported. |
The issue was I only pushed
Thanks for the word I couldn't think of when typing. 😄 Yes, it should be clear that it should exist within the source branch (not just the compiled site) on a repo. |
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.
That all being said, is there anything in particular you want me looking for? Functionally, it seems to work. I am going to take a look at restyling the CSS over the next day or so. If you feel as though the spam protection is vital enough to merge now, I approve and I will do a separate PR for styling.
Notes for commits after @pacollins's approval: The hugo-future-imperfect-slim/assets/js/staticman.js Lines 23 to 27 in 5e97233
This is due to the extra [type="hidden"] at line 52.hugo-future-imperfect-slim/assets/js/staticman.js Lines 50 to 59 in 5e97233
I had made such mistake after hours of programming, and I had forgotten that clearForm() is also called in caes of successful form submission. I had the wrong idea that the onclick listener of the "Reset" button were the only function that called clearForm() .hugo-future-imperfect-slim/assets/js/staticman.js Lines 100 to 103 in 5e97233
Now this has been corrected at ffa6711. hugo-future-imperfect-slim/assets/js/staticman.js Lines 50 to 60 in ffa6711
In addition, since a Staticman instance running on a free Heroku dyno (say, @staticmanlab) might take time to respond, I've added a SCSS rule to decrease the opacity of the submitted form by half until the submission message appears. I've changed Visual results for my demo site: https://vincenttam.frama.io/fish-demo/blog/examples/ |
Description
It is in turn a port from halogenica/beautifulhugo#222. (typo edited) Introduced true nested comments with reply functionality.
503 error using v3 endpoint eduardoboucas/staticman#307.
Motivation and Context
Resolving #63 and adding missng i18n strings to Staticman form requires changing the template, so I adapted my Staticman templates for Huginn, Introduction, Hugo Swift Theme and my blog into this theme.
Screenshots (if appropriate):
Types of changes
Checklist:
theme.toml
, accordingly.