Skip to content

feat: Google Drive Video ifrmae support #163

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: Google Drive Video ifrmae support #163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/qiita/markdown.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
require "qiita/markdown/embed/tweet"
require "qiita/markdown/embed/asciinema"
require "qiita/markdown/embed/youtube"
require "qiita/markdown/embed/google_drive_video"
require "qiita/markdown/embed/slide_share"
require "qiita/markdown/embed/google_slide"
require "qiita/markdown/embed/speeker_deck"
Expand Down
9 changes: 9 additions & 0 deletions lib/qiita/markdown/embed/google_drive_video.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
module Qiita
module Markdown
module Embed
module GoogleDriveVideo
SCRIPT_HOST = "drive.google.com".freeze
end
end
end
end
39 changes: 39 additions & 0 deletions lib/qiita/markdown/filters/google_drive_video.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
module Qiita
module Markdown
module Filters
class GoogleDriveVideo < ::HTML::Pipeline::Filter
GDRIVE_VIDEO_PATTERN = /@\[gdrive_video\]\((?<url>https?:\/\/[^\)]+)\)/

def call
doc.xpath(".//text()").each do |node|
content = node.to_html
next if !content.match?(GDRIVE_VIDEO_PATTERN)
next if has_ancestor?(node, %w(pre code tt))

html = content.gsub(GDRIVE_VIDEO_PATTERN) do
video_url = Regexp.last_match[:url]
embed_url = convert_to_embed_url(video_url)
if embed_url
%(<iframe src="#{embed_url}" width="640" height="480" frameborder="0" allowfullscreen="true"></iframe>)
Copy link

Choose a reason for hiding this comment

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

medium

The iframe dimensions width="640" height="480" are hardcoded here. I see that FilterIframe (which runs later) will change the width to "100%". However, the height="480" will persist.

Is this fixed height of 480px the desired default for all embedded Google Drive videos? If videos have different aspect ratios, this could lead to letterboxing or incorrect display.

Just wanted to confirm if this fixed height is intentional and considered appropriate for various video aspect ratios, or if there are plans or thoughts on making the height more adaptive (though that's often more complex, sometimes requiring JavaScript or CSS aspect ratio tricks).

else
Regexp.last_match[0]
Copy link
Preview

Copilot AI May 21, 2025

Choose a reason for hiding this comment

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

It appears the gsub block is missing a closing 'end' after the if/else block; please add an extra 'end' to properly close the block.

Suggested change
Regexp.last_match[0]
Regexp.last_match[0]
end

Copilot uses AI. Check for mistakes.

end
node.replace(html)
end
doc
end

private

def convert_to_embed_url(share_url)
if share_url =~ /drive\.google\.com\/file\/d\/([^\/]+)/
file_id = $1
"https://drive.google.com/file/d/#{file_id}/preview"
else
nil
end
end
Comment on lines +28 to +35
Copy link

Choose a reason for hiding this comment

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

medium

The convert_to_embed_url method could be made more robust and align better with existing patterns:

  1. URL Parsing: Instead of relying solely on regex for the share_url, consider using Addressable::URI.parse first. This would allow for more robust handling of URL components and easier validation of the host against Qiita::Markdown::Embed::GoogleDriveVideo::SCRIPT_HOST.
  2. File ID Regex: The current regex drive\.google\.com\/file\/d\/([^\/]+) captures the file ID with ([^\/]+), which is quite broad. Google Drive file IDs typically consist of alphanumeric characters, hyphens, and underscores. A more specific regex like ([-\w]+) would be safer and more precise.
  3. Capture Group Access: While $1 is functional, Regexp.last_match[1] is often preferred for clarity, especially if the method were to grow or be refactored.

Could we refactor this method to incorporate these points for better robustness and clarity? For example:

# private
#
# def convert_to_embed_url(share_url_string)
#   begin
#     uri = Addressable::URI.parse(share_url_string)
#   rescue Addressable::URI::InvalidURIError
#     return nil
#   end
#
#   return nil unless uri.host == Qiita::Markdown::Embed::GoogleDriveVideo::SCRIPT_HOST
#
#   # Match /file/d/FILE_ID/...
#   # Google Drive file IDs are typically alphanumeric with hyphens and underscores.
#   match_data = uri.path.match(/^\/file\/d\/([-\w]+)/)
#   return nil unless match_data
#
#   file_id = match_data[1]
#   "https://#{Qiita::Markdown::Embed::GoogleDriveVideo::SCRIPT_HOST}/file/d/#{file_id}/preview"
# end

This approach also centralizes the host check using the defined constant.

        def convert_to_embed_url(share_url_string)
          begin
            uri = Addressable::URI.parse(share_url_string)
          rescue Addressable::URI::InvalidURIError
            return nil
          end

          return nil unless uri.host == Qiita::Markdown::Embed::GoogleDriveVideo::SCRIPT_HOST

          # Google Drive file IDs are typically alphanumeric with hyphens and underscores.
          match_data = uri.path.match(/^\/file\/d\/([-\w]+)/)
          return nil unless match_data

          file_id = match_data[1]
          "https://#{Qiita::Markdown::Embed::GoogleDriveVideo::SCRIPT_HOST}/file/d/#{file_id}/preview"
        end

end
end
end
end
1 change: 1 addition & 0 deletions lib/qiita/markdown/transformers/filter_iframe.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ class FilterIframe
Embed::GoogleSlide::SCRIPT_HOST,
Embed::Docswell::SCRIPT_HOSTS,
Embed::Figma::SCRIPT_HOST,
Embed::GoogleDriveVideo::SCRIPT_HOST,
].flatten.freeze

def self.call(**args)
Expand Down
36 changes: 36 additions & 0 deletions spec/qiita/markdown/processor_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1520,6 +1520,42 @@
end
end

context "with HTML embed code for Google Drive Video" do
shared_examples "embed code googledrive example" do
let(:markdown) do
<<-MARKDOWN.strip_heredoc
<iframe src="#{url}" width="640" height="480" frameborder="0" allowfullscreen="true"></iframe>
MARKDOWN
end
let(:file_id) { "DRIVE_FILE_ID_EXAMPLE" }
let(:url) { "#{scheme}//drive.google.com/file/d/#{file_id}/preview" }

if allowed
it "does not sanitize embed code" do
should eq <<-HTML.strip_heredoc
<iframe src="#{url}" width="640" height="480" frameborder="0" allowfullscreen="true"></iframe>
HTML
end
else
it "forces width attribute on iframe" do
should eq <<-HTML.strip_heredoc
<iframe src="#{url}" width="100%" height="480" frameborder="0" allowfullscreen="true"></iframe>
HTML
end
end
end

context "with scheme" do
let(:scheme) { "https:" }
include_examples "embed code googledrive example"
end

context "without scheme" do
let(:scheme) { "" }
include_examples "embed code googledrive example"
end
end

context "with HTML embed code for SlideShare" do
shared_examples "embed code slideshare example" do
let(:markdown) do
Expand Down