Implement Reddit liquid tag #4221

cesc1989 · 2019-10-03T20:26:36Z

What type of PR is this? (check all applicable)

Feature

Description

Implement Liquid tag for reddit content.

It takes the post URL and renders a block of content based on the JSON response of requesting that URL and appending .json:

url =  https://www.reddit.com/r/IAmA/comments/afvl2w/im_scott_from_scotts_cheap_flights_my_profession

json_url =  https://www.reddit.com/r/IAmA/comments/afvl2w/im_scott_from_scotts_cheap_flights_my_profession.json

I tried to emulate the same way the Medium liquid tag was coded and that's why the request is made and handled in a different class(RedditJsonFromUrlService)

I'm still pending to add tests and improve the styling of the reddit content.

Related Tickets & Documents

This PR resolves #1536

Added to documentation?

docs.dev.to

What gif best describes this PR or how it makes you feel?

CLAassistant · 2019-10-03T20:27:13Z

All committers have signed the CLA.

benhalpern · 2019-10-04T13:51:19Z

Looking good so far, thanks for this

cesc1989 · 2019-10-23T00:54:06Z

Hi @benhalpern 👋

I'm close to finish this but got a couple of questions. Hope this is the place to ask.

When rendering reddit content that has image, this happens:

Don't know where in the codebase happens but seems the image URL is wrapped to make it lazy load. I'm not very sure how to proceed fix/proceed/handle this situation and would appreciate your input.

Reddit content again but with text only:

I tried to parse the markdown content but couldn't successfully do it. Again, what would be a way to solve this _in_ Dev?

Final question is this in the PR checks:

codeclimate/diff-coverage — 17% (50% threshold)

Don't understand what's going on there. I installed the codeclimate firefox plugin but can't see the diff.

I appreciate your time 😄

benhalpern · 2019-11-01T21:04:58Z

@cesc1989 I believe it is likely the images are only failing locally because you don't have Cloudinary fully set up. We can confirm this, but that is my suspicion right now.

With regards to the text/markdown/html, we should be able to parse as markdown in our normal parser.

CodeClimate is down because it needs tests. Check out spec/liquid_tags and add tests similar to what's in there for similar tags.

@maestromac @Zhao-Andy if either of you want to add some guidance or suggestions here when you could, that would be great.

maestromac · 2019-11-05T21:33:48Z

Hello @cesc1989

We lazy load all images and defer the image to Cloudinary. I've tested your branch locally and it seems to be loading the image. Is it not loading at all on your end?
We use Redcarpet and you can try to play around with it to see how it renders your Reddit's content (ie markdown = Redcarpet::Markdown.new(renderer, extensions = {}) or take a look to see how we use it here). If that doesn't work and complicate things, would it be possible to just render the plain text without the markdown?
Seems like diff-coverage is no longer the issue here

benhalpern · 2019-11-05T21:42:03Z

We lazy load all images and defer the image to Cloudinary. I've tested your branch locally and it seems to be loading the image. Is it not loading at all on your end?

@maestromac I believe this is because the cloudinary key is set to Optional in their env. So it probably works for you and not them if they don't have it set properly. I'd say this part of the PR should be good to go.

We mostly just need to proper markdown parsing now.

cesc1989 · 2019-11-06T23:17:22Z

Thanks a bunch! @benhalpern @maestromac

I finally could render the text content properly 😄 I think the PR is ready to be reviewed/merged. Let me know if there's anything to change/improve.

Zhao-Andy · 2019-11-14T19:21:14Z

Hey @cesc1989 sorry for the late review! I'm reviewing it now and noticed the icon could be more true to Reddit's icon. Do you mind if I push up a new SVG file?

Have some additional comments to make, but overall looks great!

Zhao-Andy

Left some comments. Great PR so far!

app/views/liquids/_reddit.html.erb

docs/frontend/liquid-tags.md

spec/liquid_tags/reddit_tag_spec.rb

app/services/reddit_json_from_url_service.rb

app/liquid_tags/reddit_tag.rb

app/assets/images/reddit-icon.svg

cesc1989 · 2019-11-14T23:18:38Z

@Zhao-Andy just submitted the suggested changes 😄

The reddit icon does look nicer. Thanks!

cesc1989 · 2019-11-14T23:21:23Z

BTW, @Zhao-Andy I think the code in app/views/pages/_editor_guide_text.html.erb would benefit from using more partials(maybe per tag). I freaked out when saw all that HTML with no order whatsoever 😅

How do you handle this kind of refactors?

I decided a UI similar to the Stack Exchange tag would be better. Now the Reddit tag looks a bit similar.

Also emphasize notice about compilation of article when using any liquid tag

Sometimes the attribute `post_hint` might be null and neither condition would apply.

maestromac

Sorry for the delay. Tested locally. Works great!

maestromac

Actually the size of the VCR is concerning. I need to think about a better way to test this without relying on VCR.

cesc1989 · 2020-03-18T21:42:07Z

Actually the size of the VCR is concerning. I need to think about a better way to test this without relying on VCR.

@maestromac thanks for reviewing!

I'll take a look at it and see how could it be improved.

Zhao-Andy

Overall looks good! One improvement I'd like to see i having more useful error handling. For example, if I use an invalid link here:

{% reddit https://www.reddit.com/r/aww/comments/ag3s4 %}

# errors out:
"base:  no implicit conversion of String into Integer"

Instead of a run time error, I think we'd want to raise an error like this:
https://github.com/thepracticaldev/dev.to/blob/1481faf34d5a98b259c677aa8e058e14902882ca/app/liquid_tags/gist_tag.rb#L36-L39

Sorry for the delay on all this! I'll keep a better eye on this so we can get this released sooner rather than later. :)

mstruve · 2020-05-07T20:20:44Z

Hey @cesc1989!

Thank you so much for the PR to add this incredible feature! With that said, we are in the process of going through and cleaning up some of our PR history to help us better prioritize what we need to be focusing on. Since this PR has not been active for a while and has some conflicts I am going to close it for now. If you would still like to get this merged please feel free to resolve the conflicts, make the requested changes, and then open the PR back up. We would be more than happy to help push this through if you would like!

Thanks so much for being an awesome contributor! Ping us anytime if you have questions!

cesc1989 · 2020-05-13T14:03:45Z

Thank you @mstruve for the heads up.

I have this inline to work but had it completely forgotten. Soon I'll be tackling the conflicts and the last reviews 🤠 👍

cesc1989 · 2020-05-27T22:46:58Z

Hi @mstruve @Zhao-Andy

I just pushed (to my fork branch) solved conflicts and the two feedback items provided. Do I have to open another PR?

rhymes · 2020-05-28T08:02:56Z

Hi @cesc1989, I think so, as GitHub won't let me reopen this. Make sure you're up to date with master when you create it

weekly-digest bot mentioned this pull request Oct 7, 2019

Weekly Digest (30 September, 2019 - 7 October, 2019) #4273

Closed

weekly-digest bot mentioned this pull request Oct 14, 2019

Weekly Digest (7 October, 2019 - 14 October, 2019) #4406

Closed

benhalpern requested review from Zhao-Andy and maestromac November 1, 2019 20:57

cesc1989 changed the title ~~WIP Implement Reddit liquid tag~~ Implement Reddit liquid tag Nov 6, 2019

pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Nov 6, 2019

Zhao-Andy suggested changes Nov 14, 2019

View reviewed changes

cesc1989 added 9 commits December 11, 2019 16:00

Implement Reddit liquid tag

635456f

Set reddit content in locals

41bc643

Change Reddit liquid tag content layout

840f185

I decided a UI similar to the Stack Exchange tag would be better. Now the Reddit tag looks a bit similar.

Add tests for RedditTag

3709f96

Add reddit liquid tag example to docs

e92951b

Also emphasize notice about compilation of article when using any liquid tag

Parse markdown text reddit content for RedditTag

5b8dc96

Display date RedditTag post was published

c0186c0

Remove file location comments

b5f35e1

Change order of conditional for Reddit liquid tag

08a50c0

Sometimes the attribute `post_hint` might be null and neither condition would apply.

cesc1989 and others added 3 commits December 11, 2019 16:02

Add example to use Reddit liquid tag

44a6f6e

Change reddit svg icon for Reddit liquid tag

eda37ef

Merge branch 'master' into feature/reddit-liquid-tag

7031f93

cesc1989 requested review from jacobherrington and a team as code owners March 18, 2020 19:51

maestromac requested review from Zhao-Andy and removed request for jacobherrington and a team March 18, 2020 19:51

maestromac approved these changes Mar 18, 2020

View reviewed changes

pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Mar 18, 2020

maestromac suggested changes Mar 18, 2020

View reviewed changes

pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Mar 18, 2020

Zhao-Andy suggested changes Mar 20, 2020

View reviewed changes

mstruve closed this May 7, 2020

cesc1989 mentioned this pull request May 28, 2020

Add Reddit Liquid Tag #8111

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Reddit liquid tag #4221

Implement Reddit liquid tag #4221

cesc1989 commented Oct 3, 2019 •

edited

Loading

CLAassistant commented Oct 3, 2019 •

edited

Loading

benhalpern commented Oct 4, 2019

cesc1989 commented Oct 23, 2019

benhalpern commented Nov 1, 2019

maestromac commented Nov 5, 2019

benhalpern commented Nov 5, 2019

cesc1989 commented Nov 6, 2019

Zhao-Andy commented Nov 14, 2019

Zhao-Andy left a comment

cesc1989 commented Nov 14, 2019

cesc1989 commented Nov 14, 2019

maestromac left a comment

maestromac left a comment

cesc1989 commented Mar 18, 2020 •

edited

Loading

Zhao-Andy left a comment

mstruve commented May 7, 2020

cesc1989 commented May 13, 2020

cesc1989 commented May 27, 2020 •

edited

Loading

rhymes commented May 28, 2020 •

edited

Loading

Implement Reddit liquid tag #4221

Implement Reddit liquid tag #4221

Conversation

cesc1989 commented Oct 3, 2019 • edited Loading

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

Added to documentation?

What gif best describes this PR or how it makes you feel?

CLAassistant commented Oct 3, 2019 • edited Loading

benhalpern commented Oct 4, 2019

cesc1989 commented Oct 23, 2019

benhalpern commented Nov 1, 2019

maestromac commented Nov 5, 2019

benhalpern commented Nov 5, 2019

cesc1989 commented Nov 6, 2019

Zhao-Andy commented Nov 14, 2019

Zhao-Andy left a comment

Choose a reason for hiding this comment

cesc1989 commented Nov 14, 2019

cesc1989 commented Nov 14, 2019

maestromac left a comment

Choose a reason for hiding this comment

maestromac left a comment

Choose a reason for hiding this comment

cesc1989 commented Mar 18, 2020 • edited Loading

Zhao-Andy left a comment

Choose a reason for hiding this comment

mstruve commented May 7, 2020

cesc1989 commented May 13, 2020

cesc1989 commented May 27, 2020 • edited Loading

rhymes commented May 28, 2020 • edited Loading

cesc1989 commented Oct 3, 2019 •

edited

Loading

CLAassistant commented Oct 3, 2019 •

edited

Loading

cesc1989 commented Mar 18, 2020 •

edited

Loading

cesc1989 commented May 27, 2020 •

edited

Loading

rhymes commented May 28, 2020 •

edited

Loading