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
Add twitch liquid tags #10577
Add twitch liquid tags #10577
Conversation
These will embed Twitch Clips in articles and comments.
05d418b
to
e3ce56b
Compare
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.
Tested, it works great!
I left a non blocking note about making the tests a little bit more self explanatory but great job @ChaelCodes!
I also left a question :P
def parsed_url(url) | ||
url.split(":")[0] | ||
end |
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 was going to suggest using ApplicationConfig.app_domain_no_port
but that's tied to the environment variable. Let's keep this here, I'll add a SiteConfig.app_domain_no_port
in a separate PR not to derail this one :)
|
||
# prevent param injection | ||
def parsed_slug(slug) | ||
slug.strip.split("&")[0] |
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.
FYI non blocking for-the-future: you can use .first
instead of [0]
@@ -14,6 +14,7 @@ def has_vid?(article) | |||
|
|||
article.processed_html.include?("youtube.com/embed/") || | |||
article.processed_html.include?("player.vimeo.com") || | |||
article.processed_html.include?("clips.twitch.tv/embed") || |
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.
can I ask you the reason for this change? I'm not super familiar with that part of the code, so I'm wondering why we need Fluidvids for a liquid tag if it's in an iframe, is it for the potential resizing of the container?
it "forbids inserting autoplay option" do | ||
assert_parses slug, "#{slug}&autoplay=true" | ||
end |
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.
this test is not very self explanatory. "assert it parses the slug if you pass the slug with autoplay true" but maybe we should test "make sure the slug doesn't contain autoplay=true"
end | ||
|
||
it "forbids inserting mute option" do | ||
assert_parses slug, "#{slug}&muted=true" |
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.
same as above
* Add a Liquid Tag specifically for Twitch clips. These will embed Twitch Clips in articles and comments. * Update documentation to incorporate Twitch clip liquid tags. * Include Twitch Clips in video embed logic * Making a small change to rebuild
What type of PR is this? (check all applicable)
Description
Related Tickets & Documents
resolves #10475
Twitch Documentation on Embeds
Dev.To docs on Liquid Tags
QA Instructions, Screenshots, Recordings
Please replace this line with instructions on how to test your changes, as well
as any relevant images for UI changes.
Added tests?
Added to documentation?