-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
Conversation
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.
I think AppVeyor sent the wrong status to Github: https://ci.appveyor.com/project/bep/hugo/build/1.0.69 (success here) |
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.
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! 感谢您的贡献!
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 |
Hugo 0.25 improved the embedded Disqus template so we can just use theirs now. See: gohugoio/hugo#3655 See: gohugoio/hugo#3639
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. |
@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! |
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
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. |
Set
disqus_identifier
,disqus_title
, anddisqus_url
only if the user has explicitly provided them in the metadata of a post. Two reasons: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.
If Hugo sets them, there are two problems:
disqus_identifier
is set to.Permalink
by default, which is identical todisqus_url
in the current template. This is not only redundant, but also defeats the purpose ofdisqus_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.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 hardcodedisqus_url
to.Permalink
, Disqus will continue to work, because it basically useswindow.location.href
.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.
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 isbaseurl = "/"
before other theme authors change.Permalink
to.RelPermalink
and userelURL
.