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

Embedded Discourse url problems #1182

Closed
tendon opened this Issue Aug 16, 2017 · 13 comments

Comments

Projects
None yet
2 participants
@tendon

tendon commented Aug 16, 2017

  • This is a question about using the theme.
  • This is a feature request.
  • I have updated all gems with bundle update.
  • I have tested locally with bundle exec jekyll build.
  • I believe this to be a bug with the theme --- not Jekyll, GitHub Pages or one of the bundled plugins.

Environment informations

  • Minimal Mistakes version: 4.5.0

Expected behavior

Integration with discourse as a comments provider doesn't work in all conditions.

Steps to reproduce the behavior

  1. Set up _config.yml appropriately for discourse embedding.
  2. Follow the instructions at https://meta.discourse.org/t/embedding-discourse-comments-via-javascript/31963. In particular notice the code listed in step 4, and compare it to the code generated by _includes/comments-providers/discourse.html.

There are two problems.

First, discourseUrl will be given an extra pair of forward slashes:

discourseUrl: '//http://discourse.example.com/'

while it should be:

discourseUrl: 'http://discourse.example.com/'

Second, the canonical url is missing (at least) the site.url and appears as:

discourseEmbedUrl: '/blog/entry-123.html'

while it should be

discourseEmbedUrl: 'http://example.com/blog/entry-123.html'

A fix is proposed in this PR: #1181

Note: in order for everything to work correctly, you must ensure that _config.yml contains the proper settings (site.url and site.baseurl) for the urls!

@mmistakes

This comment has been minimized.

Show comment
Hide comment
@mmistakes

mmistakes Aug 16, 2017

Owner

I believe the thinking behind discoureUrl was to feed it a protocol-less URL. SO instead of:

comments:
  provider: "discourse"
  discourse:
    server: "http://discourse.example.com/"

You give it this:

comments:
  provider: "discourse"
  discourse:
    server: "discourse.example.com"

Which is probably more of a documentation issue than an actual bug.

Owner

mmistakes commented Aug 16, 2017

I believe the thinking behind discoureUrl was to feed it a protocol-less URL. SO instead of:

comments:
  provider: "discourse"
  discourse:
    server: "http://discourse.example.com/"

You give it this:

comments:
  provider: "discourse"
  discourse:
    server: "discourse.example.com"

Which is probably more of a documentation issue than an actual bug.

@tendon

This comment has been minimized.

Show comment
Hide comment
@tendon

tendon Aug 16, 2017

But does it work if protocol-less? Would the protocol be added somehow?
(All examples I've seen - especially at https://meta.discourse.org/t/embedding-discourse-comments-via-javascript/31963 - always have the protocol in the generated JS.)

tendon commented Aug 16, 2017

But does it work if protocol-less? Would the protocol be added somehow?
(All examples I've seen - especially at https://meta.discourse.org/t/embedding-discourse-comments-via-javascript/31963 - always have the protocol in the generated JS.)

@tendon

This comment has been minimized.

Show comment
Hide comment
@tendon

tendon Aug 16, 2017

Before my patch it didn't work. What prompted the solution was this: https://meta.discourse.org/t/solved-error-embedding/57276/2.

Note the statement: "Make sure the discourseEmbedUrl is a full URL, not a relative path"

tendon commented Aug 16, 2017

Before my patch it didn't work. What prompted the solution was this: https://meta.discourse.org/t/solved-error-embedding/57276/2.

Note the statement: "Make sure the discourseEmbedUrl is a full URL, not a relative path"

@mmistakes

This comment has been minimized.

Show comment
Hide comment
@mmistakes

mmistakes Aug 16, 2017

Owner

@tendon Smells like an issue with your environment or configuration. If you are triggering the correct Jekyll production environment that path should be absolute. Your PR just swapped the absolute_url filter with site.url and site.baseurl prepends which should be the same.

If they're not and you're getting relative URLs then it's a bug with Jekyll... or perhaps you're on an older version that needs updating.

Owner

mmistakes commented Aug 16, 2017

@tendon Smells like an issue with your environment or configuration. If you are triggering the correct Jekyll production environment that path should be absolute. Your PR just swapped the absolute_url filter with site.url and site.baseurl prepends which should be the same.

If they're not and you're getting relative URLs then it's a bug with Jekyll... or perhaps you're on an older version that needs updating.

@mmistakes

This comment has been minimized.

Show comment
Hide comment
@mmistakes

mmistakes Aug 16, 2017

Owner

@tendon People like to use protocol less URLs so they can swap between both https:// and http:// (secure/insecure)... usually when building for dev/prod environments.

As far as the browser is concerned http://yourdomain.com and //yourdomain.com are the same. You'll see protcol-less URLs a lot in <script> embeds for things like jQuery on CDNs and the like.

Owner

mmistakes commented Aug 16, 2017

@tendon People like to use protocol less URLs so they can swap between both https:// and http:// (secure/insecure)... usually when building for dev/prod environments.

As far as the browser is concerned http://yourdomain.com and //yourdomain.com are the same. You'll see protcol-less URLs a lot in <script> embeds for things like jQuery on CDNs and the like.

@tendon

This comment has been minimized.

Show comment
Hide comment
@tendon

tendon Aug 16, 2017

I'll check that about the production environment. Thanks for the hint.

But doesn't //yorudomain.com try to open with the file protocol (file:///yourdomain.com)?

tendon commented Aug 16, 2017

I'll check that about the production environment. Thanks for the hint.

But doesn't //yorudomain.com try to open with the file protocol (file:///yourdomain.com)?

@mmistakes

This comment has been minimized.

Show comment
Hide comment
@mmistakes
Owner

mmistakes commented Aug 16, 2017

@mmistakes

This comment has been minimized.

Show comment
Hide comment
@mmistakes

mmistakes Aug 16, 2017

Owner

But it will if you're just opening local .html files in a browser. When opened from a file server (which is where they will eventually live) it's not an issue.

When in doubt fire up Jekyll's server with jekyll serve and test your site that way. Opening the pages directly without the WEBbrick server that Jekyll spins up is probably why you're having problems.

Owner

mmistakes commented Aug 16, 2017

But it will if you're just opening local .html files in a browser. When opened from a file server (which is where they will eventually live) it's not an issue.

When in doubt fire up Jekyll's server with jekyll serve and test your site that way. Opening the pages directly without the WEBbrick server that Jekyll spins up is probably why you're having problems.

@tendon

This comment has been minimized.

Show comment
Hide comment
@tendon

tendon Aug 16, 2017

Yes it seems ENV production or development has something to do with it; and I was checking one while I should have been looking at the other one. One possible improvement is to include the embedding only in production. Please see https://joebuhlig.com/discourse-commenting-for-jekyll/, the section "Development Tweaks".

Still trying to see if protocol-less urls work, though.

tendon commented Aug 16, 2017

Yes it seems ENV production or development has something to do with it; and I was checking one while I should have been looking at the other one. One possible improvement is to include the embedding only in production. Please see https://joebuhlig.com/discourse-commenting-for-jekyll/, the section "Development Tweaks".

Still trying to see if protocol-less urls work, though.

@tendon

This comment has been minimized.

Show comment
Hide comment
@tendon

tendon Aug 16, 2017

Yes, the protocol-less urls seem to work too. So this should be documented better.

However, I would suggest a better way is to take away the // from the assignment of discourseUrl because then it should work in all cases:

  • People who don't know about the protocol-less trick (like me before this conversation!) will include the protocol in their site.comments.discourse.server setting.
  • People who know and want to use the trick can set it explicitly there, i.e. by assigning //discourse.yourdomain.com to that same setting.

WDYT?

tendon commented Aug 16, 2017

Yes, the protocol-less urls seem to work too. So this should be documented better.

However, I would suggest a better way is to take away the // from the assignment of discourseUrl because then it should work in all cases:

  • People who don't know about the protocol-less trick (like me before this conversation!) will include the protocol in their site.comments.discourse.server setting.
  • People who know and want to use the trick can set it explicitly there, i.e. by assigning //discourse.yourdomain.com to that same setting.

WDYT?

@mmistakes

This comment has been minimized.

Show comment
Hide comment
@mmistakes

mmistakes Aug 16, 2017

Owner

My hesitation to removing the // is it's not backwards compatible and will break sites that don't specify a protocol for site.comments.discourse.server... eg. meta.discourse.org

Adding a note to the documentation that make it clearer that you shouldn't add a protocol to comments.discourse.server in _config.yml is the way to go for sure.

What I would suggest is adding some logic to the capture and test for :// in the variable. If it's there leave it alone... if not then prepend: '//'. I do something similar with images to test if they're relatively hosted or externally.

There could be some edge cases that doesn't catch so not sure if that's the best way forward either. My guess is Discourse is so rarely used with the theme it's not a big issue. Though I'd feel better if those who actually use it chime in, if the current implication needs improvement/is broken.

Owner

mmistakes commented Aug 16, 2017

My hesitation to removing the // is it's not backwards compatible and will break sites that don't specify a protocol for site.comments.discourse.server... eg. meta.discourse.org

Adding a note to the documentation that make it clearer that you shouldn't add a protocol to comments.discourse.server in _config.yml is the way to go for sure.

What I would suggest is adding some logic to the capture and test for :// in the variable. If it's there leave it alone... if not then prepend: '//'. I do something similar with images to test if they're relatively hosted or externally.

There could be some edge cases that doesn't catch so not sure if that's the best way forward either. My guess is Discourse is so rarely used with the theme it's not a big issue. Though I'd feel better if those who actually use it chime in, if the current implication needs improvement/is broken.

@tendon

This comment has been minimized.

Show comment
Hide comment
@tendon

tendon Aug 16, 2017

OK. I see the point of backward compatibility.
So you can ignore the PR, but maybe do update the documentation for future users!
Thanks.

tendon commented Aug 16, 2017

OK. I see the point of backward compatibility.
So you can ignore the PR, but maybe do update the documentation for future users!
Thanks.

@mmistakes

This comment has been minimized.

Show comment
Hide comment
@mmistakes

mmistakes Aug 16, 2017

Owner

Will do. Thanks for taking the time to start the discussion. Always good to get feedback on parts of the theme that might not be working as expected.

Owner

mmistakes commented Aug 16, 2017

Will do. Thanks for taking the time to start the discussion. Always good to get feedback on parts of the theme that might not be working as expected.

@mmistakes mmistakes closed this Aug 16, 2017

@mmistakes mmistakes reopened this Aug 16, 2017

@mmistakes mmistakes changed the title from Embedded discours url problems to Embedded Discourse url problems Aug 18, 2017

@mmistakes mmistakes closed this in 1509ef7 Aug 18, 2017

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