-
Notifications
You must be signed in to change notification settings - Fork 13
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
RedditEmbed #1754
RedditEmbed #1754
Conversation
cfe433d
to
f88d980
Compare
this implements Reddit embeds as separate blocks that are identified not through an iframe embed, but with their blockquote embed code.
for some reason the embed code doesn't have these spaces and it makes the textual content kind of unreadable
}) | ||
); | ||
continue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah what was going on here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, there's a small bug with the serialize method that was eating some text
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can separate this into another PR if that helps
}); | ||
}); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want any tests for handling Reddit URLs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we need some extra attributes for reddit content that cannot be inferred by url.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically, height :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're still identifying reddit embeds by URL above though, in utils/social-urls.ts
. Don't we want to have some tests for that? Or if we are expecting to use oembed data for this, shouldn't that be removed from the social-urls identification above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right
}> { | ||
static type = "reddit-embed"; | ||
static vendorPrefix = "offset"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reddit embeds also have an option to display in dark mode, do we want to support that option?
<blockquote
class="reddit-embed-bq"
style="height:500px"
data-embed-theme="dark"
data-embed-height="740"
>
<a href="https://www.reddit.com/r/DIY/comments/1ddx4kp/whats_going_on_with_my_washer_discharge_hose_and/">[What's going on with my washer discharge hose and how can I prevent it in the future?](https://www.reddit.com/r/DIY/comments/1ddx4kp/whats_going_on_with_my_washer_discharge_hose_and/)</a>
<br> by
<a href="https://www.reddit.com/user/Tsiah16/">[u/Tsiah16](https://www.reddit.com/user/Tsiah16/)</a> in<a href="https://www.reddit.com/r/[DIY](https://www.reddit.com/r/DIY/)/">DIY</a>
</blockquote>
<script async="" src="https://embed.reddit.com/widgets.js" charset="UTF-8"></script>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is something that is up to the calling context and not necessarily something that should be controlled by editors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still have a Q on the social-urls test for identifying Reddit URLs, but otherwise this lgtm
@bachbui just removed that bit of code, thanks for that |
Reddit has changed their embed codes from iframe embeds to a more standard blockquote + script embed code. We previously were detecting only the iframe embed versions of these and had none of the options support that Reddit provided for embedding their posts.
This will turn into: