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

Improved internal links construction #22

Merged
merged 3 commits into from Aug 6, 2019

Conversation

@VincentTam
Copy link
Collaborator

commented Aug 6, 2019

TODO:

  • resolve #17
  • resolve #21
  • i18n framework
  • Staticman nested comments (see project for details)

Supposed the uncertainty of @onweru's response time, I've chosen to bundle my resolution to the above problems together so as to minimise the possibility of merge conflicts during my future work on the later two tasks. I haven't read the entire book Pro Git, so I dunno whether this is the best Git practice. Feel free to cherry-pick any published commits inside this PR. Don't hesitate to leave a feedback/question/commit if needed.

A i18n framework is simply putting every UI string like "Designed by" into a data file under i18n. Some examples include Beautifhul Hugo and Introduction. This forms a good (though not necessary) basis for the next feature.

Nested comments of one level depth keep the comments section organised. Examples include:

  1. zcrc.me
  2. Network Hobo
  3. Mr Tortue
  4. My blog

However, in the first three examples, there's no visually clear way to represent the following common situation.

  1. main comment A
    1. O replies to A
    2. C also replies to A
    3. D replies to O
  2. main comment B

To improve this, I'll port my previous work for

into here.

Regarding backward compatibility, this should be backward compatible since missing fields (replyThread, replyID, replyName) in static comment data files won't cause a runtime error displayed in the terminal. The Go-HTML code inside an {{ if .replyThread }}...{{ end }} block is simply ignored.

P.S. Of course, site owners may manually modify these three reply* fields in their comment data file(s) to deliver an appropriate visual display according to the context, due to the site owner's total ownership of static comments—but that's out of the scope of this PR.

P.S. I'm using Atom, Sublime Text 3 and Vim for my editing, with no fixed choices. The automatic removal of trailing spaces and the automatic addition of missing EOL are due to (one of the?) former two editors. In each Git diff hunk, the three lines above and/or below each changed line are displayed. This "change" might interfere with other PRs in case of Git merges with different branches, causing a merge conflict, so that manual inspection and resolution are needed. Having good commit styles described in the commit message at 31d40fd help saving our manpower. Please consider running git diff --ws-error-highlight=all before actually staging your files in the future.

VincentTam added some commits Aug 6, 2019

Optional loading of comments section
I had omitted the first and the last diff hunks when I pushed the previous
commit to GitHub.

@VincentTam VincentTam requested a review from onweru Aug 6, 2019

@VincentTam VincentTam self-assigned this Aug 6, 2019

Unified image URL
Only one instance of `printf` remained since using `path.Join` can't solve the
problem.
@onweru

This comment has been minimized.

Copy link
Owner

commented Aug 6, 2019

Excellent work @VincentTam, I'll merge this as-is into master.

@onweru onweru merged commit a7e006d into onweru:master Aug 6, 2019

@VincentTam

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 6, 2019

@onweru Thanks for your positive feedback. I should have given a self-review like what I have done in the theme Introduction. This is one of the several themes that I'm working with, so excuse me for such omission.

  1. JS code chagne: in this PR, what I intended to change is line 230. Clicking on the pager links triggers a pop up window. This is obscured by the trailing empty spaces at a multitude of lines in the Git diff hunk. I added a condition in isExternal to eliminate anchors under .pager class.
  2. I've used relLangURL for post header images taking the future i18n into account.

Given that this PR is merged, I'll change my submission method: split i18n and Staticman nested comments into two distinct PRs.

@onweru

This comment has been minimized.

Copy link
Owner

commented Aug 6, 2019

@onweru Thanks for your positive feedback. I should have given a self-review like what I have done in the theme Introduction. This is one of the several themes that I'm working with, so excuse me for such omission.

  1. JS code chagne: in this PR, what I intended to change is line 230. Clicking on the pager links triggers a pop up window. This is obscured by the trailing empty spaces at a multitude of lines in the Git diff hunk. I added a condition in isExternal to eliminate anchors under .pager class.
  2. I've used relLangURL for post header images taking the future i18n into account.

Given that this PR is merged, I'll change my submission method: split i18n and Staticman nested comments into two distinct PRs.

Sounds good @VincentTam , could you also look at 👉 this partial? Specifically lines 1 & 8

# on line 1
style = 'background-image: url(/images/{{ .Params.image }});
...
# on line 8
<a href = '/tags/{{ . | urlize }}'
...
@VincentTam

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 6, 2019

Sounds good @VincentTam , could you also look at [this partial]? Specifically lines 1 & 8

# on line 1
style = 'background-image: url(/images/{{ .Params.image }});
...
# on line 8
<a href = '/tags/{{ . | urlize }}'
...

👌 Apart from trailing white spaces and newline at EOF, please do me another favor: please use GitHub's permalink for code snippet for a more efficient code-browsing experience. It took me several git grep to find out what is "[this partial]".

<li class = 'duo post_item'>
<a class = 'show' href='{{ .RelPermalink }}' title = '{{ .Title }}'
style = 'background-image: url(/images/{{ .Params.image }});'>
</a>
<div class = 'excerpt'>
{{ with .Params.tags -}}
<div class = 'excerpt_meta'>
{{ range first 1 . }}<a href = '/tags/{{ . | urlize }}' class = 'post_tag'>{{ . }}</a>{{ end }}

I'm quite evangelical about the separation of styles (CSS), behavior (JS) and contents (HTML). I'll try moving the inline style into assets/_component.scss.

@onweru

This comment has been minimized.

Copy link
Owner

commented Aug 6, 2019

My bad, I copied the link and forgot to write the link properly. Not that you can go back in time and undo the git grep search 😄, but I rectified the mess.

This 👇 is neat, thanks.

please use GitHub's permalink for code snippet for a more efficient code-browsing experience

@VincentTam

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 6, 2019

@onweru Sorry that I'd only tested this PR locally on a branch diverged from the master branch of this repo. When I merged this into the repo for my demo site on Framagit, relURL aren't correctly parsed: the subpath of the baseURL is missing! I'm looking into this.

onweru added a commit that referenced this pull request Aug 6, 2019

@VincentTam VincentTam referenced this pull request Aug 6, 2019
@VincentTam

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 14, 2019

Fully resovled in #36.

@VincentTam VincentTam changed the title WIP: Improved internal links construction and Staticman nested comments Improved internal links construction and Staticman nested comments Aug 14, 2019

@VincentTam VincentTam changed the title Improved internal links construction and Staticman nested comments Improved internal links construction Aug 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.