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

Fix discourse urls #1181

Closed
wants to merge 1 commit into from
Closed

Conversation

tendon
Copy link

@tendon tendon commented Aug 16, 2017

The current discourse embedding snippet was not working for me. I found two problems

  1. the discourseUrl variable hardcoded a couple of forward slashes at the beginning. The correct path should be set only with the site.comments.discourse.server configuration variable.

  2. the canonical url assigned to discourseEmbedUrl was not computed correctly.

This commit attempts to fix both.

@tendon tendon mentioned this pull request Aug 16, 2017
5 tasks
@rriemann
Copy link
Contributor

My two cents:

  • I use currently still a very old version of jekyll+this theme, i.e. I have no experience here.
  • I could imagine that the discourse canonical url must match the url in the RSS feed that is read in by Discourse. So shouldn't we create the canoncical url in the same manner like in feed.xml ?

@tendon
Copy link
Author

tendon commented Aug 16, 2017 via email

@mmistakes
Copy link
Owner

@tendon That's handled not by the theme but by jekyll/jekyll-feed plugin.

@rriemann
Copy link
Contributor

rriemann commented Aug 16, 2017 via email

@@ -1,7 +1,7 @@
{% if site.comments.discourse.server %}
{% capture canonical %}{% if site.permalink contains '.html' %}{{ page.url | absolute_url }}{% else %}{{ page.url | absolute_url | remove:'index.html' | strip_slash }}{% endif %}{% endcapture %}
{% capture canonical %}{{ page.url | replace: 'index.html','' | prepend: site.baseurl | prepend: site.url }}{% endcapture %}
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of prepending baseurl and url use the absolute_url filter which does both of these operations.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe I misconfigured something, but I am pretty sure the canonical resulted in a relative url, and that's why I got here. Go figure.
So would this be the simpler solution?:

{{ page.url | replace: 'index.html','' | absoulte_url }}

Copy link
Owner

Choose a reason for hiding this comment

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

Sure, but that's pretty much what it is now... except your version removes a few more checks.

I guess I'm waiting for someone else to verify this is actually broken and not outputting the expected URL for Discourse and not some sort of Jekyll/environment/configuration issue.

@mmistakes
Copy link
Owner

Which is pretty much what we're using now, just with a few filters to deal with index.html in paths.

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

Successfully merging this pull request may close these issues.

3 participants