Skip to content
This repository has been archived by the owner on Mar 22, 2019. It is now read-only.

Switch to Discourse comments for newer blog posts #3623

Merged
merged 7 commits into from Jan 17, 2019
Merged

Switch to Discourse comments for newer blog posts #3623

merged 7 commits into from Jan 17, 2019

Conversation

oskarrough
Copy link
Contributor

Please continue discussion here #3270.

(had to open a new PR because I accidently deleted the fork on GitHub)

@locks locks temporarily deployed to ember-website-staging-pr-3623 October 10, 2018 08:09 Inactive
@locks locks temporarily deployed to ember-website-staging-pr-3623 October 15, 2018 08:35 Inactive
@jayjayjpg jayjayjpg self-assigned this Dec 1, 2018
Copy link
Member

@jayjayjpg jayjayjpg left a comment

Choose a reason for hiding this comment

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

I just had a look already locally and the integration seems to already work great regarding sending the messages of the discourse iframe back and forth between the website and the forum ✨

@sivakumar-kailasam Do you have time to spin up a review app for this PR? Thank you 🙏

@locks locks temporarily deployed to ember-website-staging-pr-3623 December 1, 2018 16:38 Inactive
@sivakumar-kailasam
Copy link
Member

@jessica-jordan done

@jayjayjpg
Copy link
Member

Thanks to @sivakumar-kailasam this can now be tested on https://discourse-test-site.emberjs.com/blog/

I just set up the forum configuration to add the testing site to the list of CORS origins (which is currently preventing the iframe for the comments display to be embedded), but to enable CORS in the first place, we need another configuration update that doesn't seem to be able to be done in the forum admin settings themselves unfortunately. I can keep you posted once I found out who can help with this one to get this live @oskarrough

@ghost
Copy link

ghost commented Jan 11, 2019

hmm, the test link isn't resolving for me:

 curl -I https://discourse-test-site.emberjs.com/blog/
curl: (6) Could not resolve host: discourse-test-site.emberjs.com

Was just curious to test this out.

@locks locks temporarily deployed to ember-website-staging-pr-3623 January 17, 2019 15:22 Inactive
@locks locks temporarily deployed to ember-website-staging-pr-3623 January 17, 2019 16:01 Inactive
@jayjayjpg
Copy link
Member

This works well as tested on the review app: https://ember-website-staging-pr-3623.herokuapp.com/blog/2018/09/28/the-ember-times-issue-66.html. A huge thank you to @mansona who helped with unblocking the issue. This should be good to go after updating the date threshold for displaying the new Discourse comments to today's date

@locks locks temporarily deployed to ember-website-staging-pr-3623 January 17, 2019 16:33 Inactive
@locks locks temporarily deployed to ember-website-staging-pr-3623 January 17, 2019 17:16 Inactive
@@ -35,7 +35,7 @@
</div>
</article>

<% if current_article.date > Date.new(2019,1,1) %>
<% if current_article.date > Date.new(2019,0,1) %>
Copy link
Member

Choose a reason for hiding this comment

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

I guess this should be a 1 because I think Ruby is 1 indexed for dates? https://ruby-doc.org/stdlib-2.1.9/libdoc/date/rdoc/Date.html#method-c-new

I know nothing about ruby 🙈

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, thank you! I was inferring that it's the same in JavaScript - good catch ✨

Copy link
Member

Choose a reason for hiding this comment

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

That's what I thought first too, I was like "Good catch, JS it should be a 0" 😂

Copy link
Member

Choose a reason for hiding this comment

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

Updated ✨

source/layouts/blog.erb Outdated Show resolved Hide resolved
Copy link
Member

@jayjayjpg jayjayjpg left a comment

Choose a reason for hiding this comment

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

This works great, thank you for bringing in this change to the blog @oskarrough

@jayjayjpg jayjayjpg merged commit ddc3602 into emberjs:master Jan 17, 2019
@oskarrough
Copy link
Contributor Author

Yeea! Happy to see it. Thanks a lot to everyone who helped finish it!

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

Successfully merging this pull request may close these issues.

None yet

5 participants