Closed VincentTam closed 3 years ago
Haven't forgotten. Should have more free time in the coming weeks!
Initial review looks great - all of your comments seem to be good and logical. I will start a full review this evening.
Looking at #178 again I came across fancyapps/fancybox#2581 - Looks like a Vanilla JS version of Fancybox should be ready soon.
If this is true (and the only thing), then I don't feel that is necessary for this PR. Unless I am misunderstanding the problem, I don't see a way to reduce the redundancy.
Agreed and thank you for your thoughts. The "critique" is just like the antithesis and synthesis combined.
With the use of custom data-
attributes in HTML, a more "OO-friendly" approach would be:
<strong class="submit-notice" data-success_msg="{ i18n "success_msg" }" data-failed_msg="…">
tag.submit-failed
and submit-success
would be kept for readability of the CSS fileconst ApiResponseEnum = {success: 1, failed: 0}
showAlert(msg)
methodsubmit-notice
The last two points would involve playing with the strings "success" and "failed" in JavaScript. That wouldn't simplify the code. More lines would be added—IMHO, my current "redundant" approach is more readable to amateurs.
Description
HTML reorganization:
In my original PR for Staticman's nested comment support #69, I attempted to stored the post's
$threadID
with a hidden<span>
—that's not the HTML5 way. I wish I knew the use ofdata-
attributes earilier.https://github.com/pacollins/hugo-future-imperfect-slim/blob/118943abc5e94ff633a930bf0deaf628fb834459/layouts/post/staticman.html#L72-L83
Each comment is wrapped by an
<article>
tag with class namecomment
andid
received from the data file. Each comment reply is nested inside a main comment. I've added the classcomments-container
to the<div>
for JS event delegation (for details, see next point). I don't touch the commentedTODO
introduced in #154 as I don't know much about the look yet—with the fixed JavaScript, the retrieval of reply target info should be OK.I changed the tag name surrounding the Hugo code for Disqus from
<article>
to<div>
.I observed that the styles are controlled by the
post
class in the CSS, so changing the tag name toarticle
should be no impact on the styles.The quotes for HTML syntax in
layout/_defaults/comments.html
are unified to single quotes. I left the double quotes in the Hugo code untouched.assets/js/staticman.js
in Vanilla JSjQuery syntax for HTML element selection
$.(...)
replaced bydocument.querySelector(...)
in most cases.jQuery method
ready()
replaced by self-executing JS function(function(){...})();
some other method translation in the same sense:
val()
,addClass()
, etc.event delegation: Vanilla JS's
addEventListner(evt, handler)
method only allows adding listener to one single HTML element at a time, while jQuery allows adding that to a class of HTML elements$('comments').on(evt, handler)
. To write readable and maintainable code, I favored adding a new CSS class "comments-container" inlayouts/_default/comments.html
rather than using JS code to navigate the DOM structure likeform.parentNode.nextSibling
, which can be easily broken in case that some siblings are inserted in between.https://github.com/pacollins/hugo-future-imperfect-slim/blob/21d42da01277a9d42730b92b060990cda658c681/assets/js/staticman.js#L96-L104
jQuery method
ajax()
replaced with XHR.https://github.com/pacollins/hugo-future-imperfect-slim/blob/21d42da01277a9d42730b92b060990cda658c681/assets/js/staticman.js#L35-L49
jQuery method
serialize()
for converting form data to URL encoded string replaced with JSON-friendly string.https://github.com/pacollins/hugo-future-imperfect-slim/blob/21d42da01277a9d42730b92b060990cda658c681/assets/js/staticman.js#L17-L32
I've hard-coded numerical values
6
,7
and11
representing the length offields
,options
andreCaptcha][
respectively for substring extraction from form input fields' name likefields[email]
andoptions[reCaptcha][siteKey]
. The goal is to construct a JSON-friendly string like eduardoboucas/staticman#412.replaced all instances of
var
tolet
to limit the scope of the variables to inside the function only: to avoid the following variables, espaciallyurl
andapi
, from overwriting variables in other files with the same name.https://github.com/pacollins/hugo-future-imperfect-slim/blob/21d42da01277a9d42730b92b060990cda658c681/assets/js/staticman.js#L9-L15
Motivation and Context
It's motivated by the goal to replace jQuery with Vanilla in https://github.com/pacollins/hugo-future-imperfect-slim/issues/231#issuecomment-794723933. This resolves #242 and resolves #243.
Screenshots (if appropriate):
[You can use
Windows+Shift+S
orControl+Command+Shift+4
to add a screenshot to your clipboard and then paste it here.]Example
Checklist:
theme.toml
, as applicable.Critique
The JS code is repetitive. From an OO point of view, directly changing the state of the HTML element should give more concise JS code, and the l10n of button texts can be stored in CSS files. I'm not sure if
{{ i18n ... }}
can be called inassets/css/*.css
. Even though that's possible, Hugo experts might prefer putting UI text in the Go-HTML templatelayout/**/*.html
.https://github.com/pacollins/hugo-future-imperfect-slim/blob/21d42da01277a9d42730b92b060990cda658c681/assets/js/staticman.js#L66-L78