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

Improve the built-in Disqus template #3639

Merged
merged 2 commits into from
Jun 27, 2017
Merged

Improve the built-in Disqus template #3639

merged 2 commits into from
Jun 27, 2017

Conversation

yihui
Copy link
Contributor

@yihui yihui commented Jun 23, 2017

  1. Set disqus_identifier, disqus_title, and disqus_url only if the user has explicitly provided them in the metadata of a post. Two reasons:

    1. If Hugo does not set them explicitly, Disqus knows how to fetch them anyway. The shortname is the only required variable for Disqus, and all other variables are optional.

    2. If Hugo sets them, there are two problems:

      1. disqus_identifier is set to .Permalink by default, which is identical to disqus_url in the current template. This is not only redundant, but also defeats the purpose of disqus_identifier: the identifier is introduced to instruct Disqus to load the same discussion thread for a page even if its URL has been changed. In other words, it is supposed to be a fixed value, independent of the page URL. Ideally this should be provided by the user explicitly.

      2. disqus_url is redundant because Disqus also uses .Permalink as its default value. There is one scenario in which Hugo's .Permalink will fail Disqus: baseurl = "/" in config.toml.1 In this case, .Permalink does not contain the full URL (e.g., /2017/06/23/hello-world/). However, if you do not hardcode disqus_url to .Permalink, Disqus will continue to work, because it basically uses window.location.href.

  2. Do not load Disqus when the website is previewed locally, otherwise it is very confusing and even frustrating (e.g. comments sent to an unexpected URL by accident).

I believe these two changes will greatly improve the user experience with Disqus for those who just uses the default Disqus template in Hugo.


  1. In case you wonder why I set baseurl = "/": I know the disadvantages, e.g., sitemap.xml won't work, but I want all URLs on my website to be portable, i.e., not tied to my domain. The solution is .RelPermalink in the theme/templates but most theme authors just use .Permalink and I cannot fix these themes. The most important reason for making URLs portable is Netlify's preview for pull requests: the best way to make the preview websites work is baseurl = "/" before other theme authors change .Permalink to .RelPermalink and use relURL.

Set `disqus_identifier`, `disqus_title`, and `disqus_url` only if the user has explicitly provided them.

Do not load Disqus when the website is previewed locally, otherwise it is very confusing.
@CLAassistant
Copy link

CLAassistant commented Jun 23, 2017

CLA assistant check
All committers have signed the CLA.

@yihui
Copy link
Contributor Author

yihui commented Jun 23, 2017

I think AppVeyor sent the wrong status to Github: https://ci.appveyor.com/project/bep/hugo/build/1.0.69 (success here)

@anthonyfok anthonyfok merged commit 2e1e493 into gohugoio:master Jun 27, 2017
Copy link
Member

@anthonyfok anthonyfok left a comment

Choose a reason for hiding this comment

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

Hi @yihui,
You seem to have a lot of experience working with Disqus, and quite an expert in this area, so I'll take your word for it. :-) Merged.

Thank you for your contribution! 感谢您的贡献!

@yihui yihui deleted the patch-1 branch June 27, 2017 13:27
@yihui
Copy link
Contributor Author

yihui commented Jun 27, 2017

Thanks for merging my first PR to Hugo! 🎉 Yes, I have used Disqus for quite a few years, and I think I'm fairly familiar with Disqus. Since you were okay with this PR, I'll submit another one shortly, which will not be as important as this one but will make the code a little bit cleaner.

And just in case anyone run into problems after this PR, below is the original template code:

{{ if .Site.DisqusShortname }}<div id="disqus_thread"></div>
<script type="text/javascript">
    var disqus_shortname = '{{ .Site.DisqusShortname }}';
    var disqus_identifier = '{{with .GetParam "disqus_identifier" }}{{ . }}{{ else }}{{ .Permalink }}{{end}}';
    var disqus_title = '{{with .GetParam "disqus_title" }}{{ . }}{{ else }}{{ .Title }}{{end}}';
    var disqus_url = '{{with .GetParam "disqus_url" }}{{ . | html  }}{{ else }}{{ .Permalink }}{{end}}';
    (function() {
        var dsq = document.createElement('script'); dsq.type = 'text/javascript'; dsq.async = true;
        dsq.src = '//' + disqus_shortname + '.disqus.com/embed.js';
        (document.getElementsByTagName('head')[0] || document.getElementsByTagName('body')[0]).appendChild(dsq);
    })();
</script>
<noscript>Please enable JavaScript to view the <a href="http://disqus.com/?ref_noscript">comments powered by Disqus.</a></noscript>
<a href="http://disqus.com" class="dsq-brlink">comments powered by <span class="logo-disqus">Disqus</span></a>{{end}}

If it is desirable for a user to revert to the old template, the above code can be saved to layouts/partials/disqus.html. Then replace {{ template "_internal/disqus.html" . }} with {{ partial "disqus.html" . }}.

alanorth added a commit to alanorth/hugo-theme-bootstrap4-blog that referenced this pull request Jul 7, 2017
Hugo 0.25 improved the embedded Disqus template so we can just use
theirs now.

See: gohugoio/hugo#3655
See: gohugoio/hugo#3639
@tombennet
Copy link
Contributor

Hey @yihui - thanks for this contribution. Am I right in thinking that this change negates the advice in the Hugo docs on conditionally loading Disqus? Given that the built-in Disqus template now does not load on localhost, the suggestion that users create a separate partial seems redundant. If so, let me know - I'm happy to submit a PR updating the docs.

@yihui
Copy link
Contributor Author

yihui commented Jul 18, 2017

@tombennet Yes, you are right. The docs should be updated, and there is no need to use a separate partial only for this purpose. Thanks!

bep added a commit that referenced this pull request Oct 19, 2017
e65df1059 Bump to v0.30
e9e118730 releaser: Prepare repository for 0.31-DEV
e6f2508d4 releaser: Add release notes to /docs for release of 0.30
9c5d6a65b releaser: Bump versions for release of 0.30
88bf0e663 Merge commit 'ecf5e081b5540e69f4af330233f39a07baf53846'
6c7191331 Merge commit 'dae5a7c61cceeb0de59f2d755f63e453f71dd9b2'
efd1821bd tpl: Add errorf template function
0cf8dc046 Change SummaryLength to be configurable (#3924)
9e8c09652 tpl: Add os.fileExists template function
e969cfcd7 Merge commit '9d68f695e782c6a83c77aff13317c7a22c694c98'
1a2d516a0 tpl: Add float template function
5310162dc releaser: Prepare repository for 0.30-DEV
648fdf2d0 releaser: Add release notes to /docs for release of 0.29
b2f46992c releaser: Bump versions for release of 0.29
ec447e043 releaser: Prepare repository for 0.29-DEV
9f469e93a releaser: Add release notes to /docs for release of 0.28
c91c18ebc releaser: Bump versions for release of 0.28
253d2ede2 Merge commit '61c27b58b353c73772aae572c7d822fdfdf7791b'
e35b93cc7 Merge commit '30694a133a88d5f76a51d0372646e10cbeca7691'
9fad59f66 Merge commit '7a89dce53bfbd67a17442a8f9be8fa895fc4f9b1'
4221c2855 Merge commit 'ba45da9d03056447e4873de13d4e0f8d658a769b'
0010b6743 releaser: Prepare repository for 0.28-DEV
9e71765cd releaser: Add release notes to /docs for release of 0.27.1
56206b90d releaser: Bump versions for release of 0.27.1
59522fca0 releaser: Prepare repository for 0.28-DEV
4686686d2 releaser: Add release notes to /docs for release of 0.27
0cd9a5d58 releaser: Bump versions for release of 0.27
d21a59d2d docs: Merge commit '1b4319be62ba071f79e90ef32dbe92eb893429f7'
9c4ff2d8e docs: Document Related Content
887fb1af7 docs: Merge commit '7d63a23b0c68d9cd7c7c09c2755619237bc03485'
c8163b51b Update docs versiona and README
f782c9959 Merge commit 'ec4e6f9df2ab9ffdc62a3f59675369096e0d3f77' as 'docs'
d384c66 docs: Re-integrate
9a2eb0f Revert "Squashed 'docs/' changes from 35abbc86..f887bd7b"
0373e43 Squashed 'docs/' changes from 35abbc86..f887bd7b
ea2cc26 Remove the theme submodule from /docs
0f9f73c Add support for multiple config files via --config a.toml,b.toml,c.toml
c8257f8 Render task list item inside label for correct accessibility
0abdeee source: Normalize UniqueID between Windows & Linux
e2f8664 hugolib: More spelling
46ac745 all: Fix spelling
4b54fb0 all: gofmt -s
40d7d3b releaser: Prepare repository for 0.27-DEV
f090c27 releaser: Add release notes to /docs for release of 0.26
b36f6e3 releaser: Bump versions for release of 0.26
0f51e49 releaser: Add release notes draft for 0.26
62583db vendor: Update checksum for inflect
0d495d5 releaser: Update to new release notes location
22b213b Merge commit 'e81208265bb3cdb7606d051a23d83aeebcb7d34d'
e812082 Squashed 'docs/' changes from ef02e34e..35abbc86
11e5d45 releaser: Include stats from hugoDocs
f768c27 helpers: Remove some unused funcs
81c1317 Add some missing doc comments
9891c0f Remove sourceRelativeLinks
481924b helpers: Fix broken TaskList in Markdown
09907d3 Switch from fork bep/inflect to markbates/inflect
8fb594b Make the title case style guide configurable
9b4170c Remove unused dependencies from vendor.json
6acbe41 media: Add missing JSON tags to Type
e321306 media: Add JSON tags to Type
9c19778 output: Add JSON tags to Format
50ec65f Squashed 'docs/' changes from 73f355ce..ef02e34e
1c18f3f Merge commit '50ec65fbe1a48475d3320775dab2c47389c02114'
cb9dfc2 helpers: Add support for French Guillemets
c4a0b6e vendor: Add support for French Guillemets
a8080c0 Remove CODEOWNERS
84710eb Add -u flag for go get govendor in CONTRIBUTING.md
555a9bc tpl: Accommodate gccgo in TestMethodToName
55d0b89 tpl/collections: Fix intersect on []interface{} handling
aee2b06 Add --debug option to be improved on over time
c1a5da9 vendor: Update dependencies for 0.26-DEV
9ed48c1 Dockerfile: Run go install with -ldflags '-s -w'
bfe0bfb Dockerfile: Reduce image size from 277MB to 27MB
606d6a8 Dockerfile: Optimize Docker image size
12e0495 docs: Add RSS template lookup example
6cd33f6 tpl: Use hash for cache key
dbe6397 hugolib: Support reflinks starting with a slash
0c90e6d Change "hugodocs" to "hugoDocs" to match GitHub's default URL
b60aa1a helpers: Add --trace to asciidoctor args
ff433f9 Add script to pull in docs changes
2c0d1cc Squashed 'docs/' changes from b0470688..73f355ce
f387cb1 doc: Merge commit '2c0d1ccdcd95de0bddeb39dca2e4d08f0d8056d7'
40566ec Remove ^M from file to make line-endings consistent
7759a98 Clarify the repo choice in the contribution guidelines
720786c Add note about doc-related pull requests in contribution guide
214e16e appveyor: Update submodules
a2fb815 Add some README info about the docs repo
73273d4 Add the docs theme to .gitmodules
4c220c4 Merge commit '6dbde8d731f221b027c0c60b772ba82dad759943'
6dbde8d Squashed 'docs/' changes from f3c88b08..b0470688
deccc54 hubolib: Add HasShortcode
00b590d Improve the twitter card template
ea5e9e3 Add GOEXE to support building with different versions of `go`
61bb3cc hugolib: Improve panic handling in layout rendering
794ea21 hugolib: Make template panics into nice error messages
91f410e Bump versions to 0.26-DEV
0e25f1e Revert "Revert "vendor: Update dependencies for 0.26-DEV""
aded65b releaser: Prepare repository for 0.25-DEV
bbd33db releaser: Add release notes to /docs for release of 0.25.1
0e09be7 releaser: Bump versions for release of 0.25.1
195f945 releaser: Ignore openbsd/arm
44b8f74 releaser: Add release notes draft for 0.25.1
a48e132 Revert "vendor: Update dependencies for 0.26-DEV"
7f82b41 parser: Final (!) fix for issue with escaped JSON front matter
84db6c7 parser: Fix issue with escaped JSON front matter
e0cf2e0 tpl/collections: Add some empty slice tests to intersect
dbbc5c4 tpl/collections: Fix union when the first slice is empty
7bcc1ce commands: Navigate to changed on CREATE When working with content from IntelliJ IDE, like WebStorm, every file save is followed by two events: "RENAME" and then "CREATE".
fd41e70 Add first draft of CODEOWNERS
4ec8ee7 vendor: Update dependencies for 0.26-DEV
1e19a98 releaser: Prepare repository for 0.26-DEV
6fb5563 releaser: Add release notes to /docs for release of 0.25
51bcd50 releaser: Fix tag push
6e086e3 releaser: Bump versions for release of 0.25
5895e32 releaser: Add release notes draft for 0.25
b2dcd86 Revert "vendor: Update dependencies"
d2c24ba vendor: Update dependencies
4989950 releaser: Use real theme count in template
a358b33 docs: Regenerate the docs helpers
a392fca docs: Regenerate commands doc
75f782f Revert "commands: Adjust docs path"
37c6905 Squashed 'docs/' changes from 5d9a6703..f3c88b08
e00f5c9 docs: Merge docs commit '37c69054e294bf554be03cf7e4c01e1f586e6943'
4776840 releaser: Adjustments
70d8ddc releaser: Revise the docs handling to match new subtree
5f831a6 releaser: Replace the magic version handling
e7a54e7 releaser: Add --try flag to ease testing
aa6b1b9 output: Support templates per site/language
a1d260b hugolib: Extend the sections API
dd9b1ba hugolib: Make .Site.Sections return the top level sections
1039356 commands: Support human-readable YAML boolean values in undraft
ccdd08d tpl/collections: Add Pages support to Intersect and Union
d12cf5a tpl/collections: Fix In function for JSON arrays
e10e51a parser: Fix handling of JSON front matter with escaped quotes
34c5667 tpl/math: Add log function
41805dc hugolib: Render 404.html for all languages
7ee1f25 hugo import jekyll support nested _posts directories
3aa0e16 cache: Add even more concurrency to test
b3c8056 cache: Add concurrent cache test
fe132e1 vendor: Add missing WebP dependencies
8431c8d tpl: Add WebP images support
da72805 tpl: Only show post's own keywords in schema.org
72fd871 hugolib: Add more summary test
118f8f7 Dockerfile: Update Dockerfile and add Docker optimizations
56d82aa hugofs: Fix typo in code comment
eccb064 tpl: Simplify the Disqus template a little bit (#3655)
2e1e493 tpl: Improve the built-in Disqus template (#3639)
a544049 helpers: Add Blackfriday 'joinLines' extension support (#3574)
bfce30d helpers: add --initial-header-level=2 to rst2html (#3528)
30e14cc Make `--navigateToChanged` more robust on Windows
c825a73 Support open "current content page" in browser
7198ea8 Revert "Remove docs building from CI builds"
dd78d5b Squashed 'docs/' content from commit 5d9a6703
a7765bb Merge commit 'dd78d5b23fe597f4461aa4199401b4e07c0612e2' as 'docs'

git-subtree-dir: docs
git-subtree-split: e65df1059549d951a45853576374be4088ded1d3
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants