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
Added YouTube video ID to talks and embedded videos for past talks #281
Added YouTube video ID to talks and embedded videos for past talks #281
Conversation
Should I presume from the above coverage decrease alert that that PR will not be accepted on those grounds? |
Yep. I can do the review in parallel, but coverage requirement has to be met. |
No problem. I'll add some coverage tonight. |
@@ -9,6 +9,11 @@ | |||
|
|||
<h4><%= render_markdown_as_html "#{talk.title.titleize}, by #{talk.member.name}" %></h4> | |||
|
|||
<% if talk.event&.starts_at&.past? && talk.youtube_id.present? %> | |||
<iframe src="https://www.youtube.com/embed/<%= talk.youtube_id %>?rel=0&autoplay=0" frameborder="0" allowfullscreen width="640" height="360"></iframe> |
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 pull-request is pretty simple and very good. The only suggestion I would make is create a helper for the youtube tag
I'm merging, even with my suggestion. I understand that you might not have the time to make that change and this is adding a lot of value to the project as is. Thank you for opening that pull-request! |
The pull request introduces embedded YouTube videos for past talks as proposed in #277.
This feature does the following:
youtube_id
string column to the Talk modelyoutube_id
field to the talk dashboard<iframe>
video if the talk is both in the past and has a non-nilyoutube_id