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

add disqus_url to comment js block #669

Closed
wants to merge 3 commits into from

Conversation

@imley
Copy link
Contributor

imley commented Jan 5, 2013

By default, the disqus_url is not set in the disqus_thread block, which is suggested to be set in the disqus official docs.

With disqus_url not set, if I opened my article in the browser in a dev environment before publishing, the page is rendered with url such as http://localhost:8000/your-slug/, and disqus will use that url as the disqus_url(explained in the doc).

Once set, the thread will use that disqus_url to generate links to the comments, that is, every comment, no matter put on product environment or dev env, will be set with the localhost:8000 started url. What's more, the disqus_url cannot be updated once set, you'll have the change the identifier(slug) to get rid of that localhost:8000 url.

So, I change the article template of not my idea theme, and I recommend other theme maker to add the disqus_url settings, too.

imley added 2 commits Jan 5, 2013
@justinmayer

This comment has been minimized.

Copy link
Member

justinmayer commented Mar 4, 2013

Hi Leroy. Thanks for your contribution, which seems like a good practice. A few comments...

I'm not sure that FEED_DOMAIN is relevant in this context; it's only purpose is for folks who host their feeds on a separate (sub-)domain as their site (e.g., feeds.example.com versus example.com, respectively). In order to construct a fully-qualified URL, I believe the only relevant variable is SITEURL.

In summary, could we not simply do the following to include the disqus_url?

{% if SITEURL %}
    var disqus_url = "{{ SITEURL }}/{{ article.url }}";
{% endif %}

And actually, this brings up a topic that I've brought up on IRC before... Should we not simply disable the entire Disqus comment <div> when SITEURL is undefined? What possible good can come of generating Disqus threads with URLs containing http://localhost:8000/?

Instead of the following in themes/notmyidea/templates/article.html...

{% if DISQUS_SITENAME %}

... why not something like this?

{% if DISQUS_SITENAME and SITEURL and article.status != "draft" %}

If that were the default, the scope of this pull request could be changed to adding a single line:

var disqus_url = "{{ SITEURL }}/{{ article.url }}";

What do you think?

@imley

This comment has been minimized.

Copy link
Contributor Author

imley commented Mar 5, 2013

Thanks for your reply.

I agree with you that FEED_DOMAIN is a bad choice here, the reason I am using FEED_DOMAIN is that using SITEURL when RELATIVE_URLS is set will generate a relative url, which is not recommended in DISQUS. I don't know what else I can use to generate the real SITEURL( FEED_DOMAIN is set as SITE_URL when not specified, in which case will work for me -- but not a good idea when it is specified differently from SITEURL).

Other than that, I think the way you suggested are much more clean and should work the same.

@justinmayer

This comment has been minimized.

Copy link
Member

justinmayer commented Mar 5, 2013

Actually, you're quite right... Since I don't use the RELATIVE_URLS setting, I had no idea that it actually overrides the SITEURL variable contents and substitutes a relative reference, leaving us (to my knowledge) without a way to generate canonical URLs from within the template context when RELATIVE_URLS = True. Crikey.

I'll bring this up with some other folks and solicit some ideas.

@justinmayer

This comment has been minimized.

Copy link
Member

justinmayer commented Mar 5, 2013

After a brief discussion in IRC, I suspect we'll resolve this (for now) by changing the default value for RELATIVE_URLS from True to False and letting folks know that they shouldn't enable that setting unless they understand its limitations. That way, we should be able to count on consistent SITEURL behavior in the template context.

@imley

This comment has been minimized.

Copy link
Contributor Author

imley commented Mar 5, 2013

Thanks, Justin. I've updated the code according to your suggestion, this should work fine if users set RELATIVE_URLS as False, and if they set it True and a relative url is generated, it seems that DISQUS would simply ignore it.

@almet

This comment has been minimized.

Copy link
Member

almet commented Mar 6, 2013

thanks, fixed in 4f6467c

@almet almet closed this Mar 6, 2013
@Shaked

This comment has been minimized.

Copy link

Shaked commented Feb 12, 2014

Hey @ametaireau is it enough to use SITEURL ? I have just tried to use this solution in my template while using:

ARTICLE_SAVE_AS = '{date:%Y}/{date:%b}/{date:%d}/{slug}.html'
ARTICLE_URL = '{date:%Y}/{date:%b}/{date:%d}/{slug}.html'

It seems that SITEURL will end up as "../../../" instead of an empty string "", therefore I have used the following solution:

{% if DISQUS_SITENAME and True != RELATIVE_URLS and article.status != "draft" %} 

Which seems to work, what do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.