Skip to content

Conversation

jessleenyc
Copy link
Contributor

@jessleenyc jessleenyc commented Apr 2, 2019

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

  • Refactor
  • Feature
  • Bug Fix
  • Documentation Update

Description

Adds a page (/videos) to see all posts w. native video content.

@jessleenyc jessleenyc changed the title [WIP]Jess/videos [WIP] Jess/videos Apr 2, 2019
@@ -0,0 +1,3 @@
<% @video_articles.each do |video_article| %>
<%= render "articles/video_player", meta_tags: false, article: video_article %>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good thought, but we've switched on the main feed to just showing the image which click through and land you on the page, rather than the literal player, which is a pretty heavy burden on the page.

This is the index code, note the video url as the BG image and the video-camera.svg icon and duration. The icon is probably not necessary on a page made only of images.

  <% if story.video.present? && story.video_thumbnail_url.present? %>
    <a class="single-article-video-preview" style="background-image:url(<%= story.cloudinary_video_url %>)" href="<%= story.path %>">
      <div class="single-article-video-duration">
        <img src="<%= asset_path("video-camera.svg") %>" /><%= story.video_duration_in_minutes %></div>
    </a>
  <% end %>

So basically this will be a grid full of videos and their titles under them. We can fiddle with design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh! that makes much more sense than having them be play-able. I don't know why I was trying to do that, esp cause the controls weren't showing up. Glad I pushed this sooner than later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

class VideosController < ApplicationController
after_action :verify_authorized
after_action :verify_authorized, except: %i[index]

Copy link
Contributor

Choose a reason for hiding this comment

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

Check out the pages_controller for the simplest example of applying edge caching to the page. (set_cache_control_headers and set_surrogate_key_header)

Then the page will need to be busted at appropriate times, like any other page.

resources :events, only: %i[index show]
resources :additional_content_boxes, only: [:index]
resources :videos, only: %i[create new]
resources :videos, only: %i[index create new]
Copy link
Contributor

Choose a reason for hiding this comment

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

lgtm

@jessleenyc jessleenyc changed the title [WIP] Jess/videos "/videos" Apr 5, 2019
@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Apr 5, 2019
Copy link
Contributor

@benhalpern benhalpern left a comment

Choose a reason for hiding this comment

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

Overall nicely done to fit the style of how things have been done in the past.

I made some comments about details that will need to be changed, but since this is a page with no links to it yet, I think it's fine to push it and then make the changes after.

end

def index
@video_articles = Article.where.not(video: nil, video_thumbnail_url: nil).where(published: true).order("published_at DESC").page(params[:page].to_i).per(12)
Copy link
Contributor

Choose a reason for hiding this comment

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

We will also need to add a set_surrogate_key_header call here to go along with set_cache_control_headers above.

Full details her: https://github.com/fastly/fastly-rails

def create
authorize :video
@article = ArticleWithVideoCreationService.new(article_params, current_user).create!
CacheBuster.new.bust "/videos"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should probably go in the general article cachebusting spot under the conditional that the article has a video.

This is because the create event here doesn't correlate with a new video being published, which is tied to the article's usual lifecycle.

expires_in: 10.minutes
respond_to :json

caches_action :show,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is left over from a copy and paste, eh? No show action in this controller.

@pr-triage 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 Apr 5, 2019
@benhalpern benhalpern merged commit 175569f into forem:master Apr 5, 2019
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Apr 5, 2019
@jessleenyc jessleenyc deleted the jess/videos branch July 29, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants