サムネイル画像をOGP画像用のサイズに合わせる #6483
サムネイル画像をOGP画像用のサイズに合わせる #6483
Conversation
wata00913
force-pushed
the
feature/fit-to-size-for-ogp-thumbnails
branch
from
9b59181 to
8770428
Compare
|
@syo-tokeshi |
@wata00913 調査して、ちゃんとレビューするようにします😊 |
|
@syo-tokeshi
明後日でなくとも、ゆっくり返信して下さって大丈夫ですよ！ 🙆♂️（GWですしね） |
| @@ -32,6 +32,8 @@ def create | |||
| @article.user = current_user if @article.user.nil? | |||
| set_wip | |||
| if @article.save | |||
| Ogp::Image::AttachmentProcessor.fit_to_size(@article.thumbnail) if @article.thumbnail.attached? | |||
There was a problem hiding this comment.
fit_to_size
こちら、リサイズしようとしているので、他の名前が良いと思いました！
私が考えた候補として、
resize_to_standard_sizeresize
です。
resizeだけでも良い気がしてきました、、(standard_sizeという言葉を付けると、何を基準に標準？って疑問が生まれる気がしました)
There was a problem hiding this comment.
resizeの場合、単純に画像のサイズを変更するだけなので命名として物足りないかなぁと考えております。
こちらのメソッドは、サムネイル画像をOGP画像として表示するために最適な加工処理を行うものです。最適な加工処理とは、今回の場合はリサイズすることと切り抜きになります。
fitは、語源の「整える」から 適合する、 適した 、 ぴったり合う という意味になります。
そのため、メソッドの役割が「OGP画像に適するように画像を加工する」ことを表現するのにfitを採用するのが妥当と判断しています。
また、画像をあるスペースを満たすように、つまりあるスペースに適するように画像を加工するCSSのプロパティがあります。プロパティ名は object-fit で fitを採用しています。
object-fit - CSS: カスケーディングスタイルシート | MDN
命名に fit_to_size と明示的に size を加えましたが、OGP画像の表示領域に合わせるという意味は fit だけでも伝わるのかなとも思いました。
There was a problem hiding this comment.
かしこまりました。
fit_to_sizeが一番ピッタリきそうですね😊
勉強になりました🙇♂️
app/models/ogp/image.rb
Outdated
| WIDTH = 1200 | ||
| HEIGHT = 630 | ||
|
|
||
| def fit?(width:, height:) |
There was a problem hiding this comment.
こちら、just_fit?というメソッド名など、どうでしょうか？
fitだけだと、「ちょうど(1200px x 630px)」が求められている事が分からない気がしました🙇♂️
There was a problem hiding this comment.
確かにjust_fitの方が1200px x 630pxが求められている事が伝わると思いました！
| module Ogp | ||
| module Image | ||
| class AttachmentProcessorTest < ActiveSupport::TestCase | ||
| test '.fit_to_size' do |
There was a problem hiding this comment.
こちらも、どのような
- width
- height
が来ても、「1200px x 630px」にリサイズされる事を確認するテストなので、
名前を変えた方が良いと思いました🙇♂️
resize_blob
はどうでしょうか？
一枚一枚の写真(画像)の事を、Active Storageでは「blob」と表現しており、
それをリサイズしているからです
There was a problem hiding this comment.
resizeについては、こちらのコメントに書きました〜
resize_blobはどうでしょうか？
今回の実装では、画像はモジュール名Ogp::Imageで表現しています。そのため、メソッド名の目的語に画像を含まなくても問題ないと判断しました。
Active Storageでは「blob」と表現しており
自分はこのことを知りませんでした。どちらで言及しているのか教えて頂けないでしょうか？🙏
一般的にblobはbinary large objectの略で画像や音声等のバイナリデータを格納するデータ型になります。ですので、blobだと画像以外も含まれ、かつ画像や音声そのものを示さないです。
ActiveStorage::Blobのblobは、画像、ビデオファイルに関するメタデータとファイルがどこのサービス上に存在するか示すキーを含むレコードとして表現しているようです。
There was a problem hiding this comment.
自分はこのことを知りませんでした。どちらで言及しているのか教えて頂けないでしょうか？🙏
すみません、自分の解釈ミスでした🙇♂️
wataさんのおっしゃられる通り、
blob = バイナリ・ラージ・オブジェクトであり、
一枚一枚の写真(画像)の事を「blob」とは呼びませんね、、🙇♂️
訂正したします🙇♂️
test/models/ogp/image_test.rb
Outdated
| extend Ogp::Image | ||
| end | ||
|
|
||
| test '.fit_to_size_option' do |
There was a problem hiding this comment.
[没案❌]
こちら、下のresize_option メソッドで、どのような
- width
- height
を渡しても、規定値の
- WIDTH = 1200
- HEIGHT = 630
のどちらかを、返却する事を確認するテストがあるので、
このテストと上手く統合したいと思いました🙇♂️
[このままで良いと思った理由🟢]
↑最初、このように考えましたが、
このテストは、正常系テストなので、このままで良いと思いました。
下に、resize_optionメソッドを、同値分割法でテストを行っているので、
このままで良いと判断しました
test/models/ogp/image_test.rb
Outdated
| assert_equal expected.to_a, Dummy.fit_to_size_option(width: 500, height: 500).to_a | ||
| end | ||
|
|
||
| test '.fit?' do |
There was a problem hiding this comment.
こちらも、https://github.com/fjordllc/bootcamp/pull/6483/files#r1181512908で指摘した通り、
just_fit?という名前が良いと思いました🙇♂️
There was a problem hiding this comment.
ご指摘ありがとうございます！こちらも同様にjust_fit?の方が意味が明確に伝わると思いました！
| @@ -41,6 +43,8 @@ def create | |||
| def update | |||
| set_wip | |||
| if @article.update(article_params) | |||
| Ogp::Image::AttachmentProcessor.fit_to_size(@article.thumbnail) if @article.thumbnail.attached? | |||
There was a problem hiding this comment.
上のメソッド名を変えると、こちらも変更になる感じですかね🙇♂️
test/models/ogp/image_test.rb
Outdated
| assert_not Dummy.fit?(width: 1200, height: 631) | ||
| end | ||
|
|
||
| test '.resize_option return 1200x when width magnification is grater than height magnification' do |
There was a problem hiding this comment.
下のテストも合わせ、
パターンは問題なく網羅されていると思いました🙇♂️
There was a problem hiding this comment.
細かく確認して頂きありがとうございます！
|
@wata00913 レビューをする過程でたくさん勉強になりました🙇♂️ |
|
@syo-tokeshi レビュー指摘箇所に対してコメントしました。命名についてのコメントで何かご指摘があればよろしくお願いします。 暫くの間、コメントへの返信が遅れるかもしれないです。申し訳ありません。🙇♂️ |
|
@wata00913 レビューをする中で、圧倒的にwataさんの方が考えたのだろうな、というのが分かり、 こうやってレビュアーと会話する中で、命名一つ取っても、
かしこまりました！ |
wata00913
force-pushed
the
feature/fit-to-size-for-ogp-thumbnails
branch
from
2996f22 to
81dc2a5
Compare
|
@syo-tokeshi ご指摘頂いた箇所を修正しましたので、確認をよろしくお願いします🙏 |
app/models/ogp/image.rb
Outdated
| @@ -5,7 +5,7 @@ module Image | |||
| WIDTH = 1200 | |||
| HEIGHT = 630 | |||
|
|
|||
| def fit?(width:, height:) | |||
| def just_fit_to_size?(width:, height:) | |||
There was a problem hiding this comment.
変更ありがとうございます😊
４つの単語で構成されていますが、一つ一つの単語が短いこともあり、これで良さそうですね😆
意味が明確になって良くなったと思います😊
|
|
||
| article.thumbnail.blob.analyze unless article.thumbnail.blob.analyzed? | ||
| assert_equal({ width: 1200, height: 630 }, | ||
| assert_equal({ width: WIDTH, height: HEIGHT }, |
There was a problem hiding this comment.
こちら悩みますね、、
読んでスッキリ意味が分かるのは、前回のこちらの気がします、、
assert_equal({ width: 1200, height: 630 }
前回の方が良い気がしてます、、🙇♂️
テストが行われているこのファイル自体に
- WIDTH
- HEIGHT
の値があるなら、問題ないと思うのですが、app/models/ogp/image.rbで定義されていて、ジャンプしないと見れないのが気になりました、、🙇♂️
また、
{ width: WIDTH, height: HEIGHT }上記はデータ型としてハッシュですが、
キーとバリューが同じ文言になっているのもちょっと引っかかってしまいます、、(他でもそういうのを見かけますが、今回のケースだと何故か違和感を感じてしまう)
{ width: 1200, height: 630 }こっちの方が、違和感がない気がします
There was a problem hiding this comment.
問題ないと思うのですが、app/models/ogp/image.rbで定義されていて、ジャンプしないと見れないのが気になりました、、🙇♂️
確かに、テストコード内で定義したものか、モジュールで定義したものか判断しづらいですね！
別のテストコードでもハードコーディングしてるので、元に戻そうと思います〜
|
コメント致しました🙇♂️ 今回のケースでは、可読性を考慮して前回のコード( ベタ書きを採用しても良いような気がします。 |
|
@syo-tokeshi |
|
@wata00913 |
|
@syo-tokeshi @komagata |
app/models/ogp/image.rb
Outdated
| # frozen_string_literal: true | ||
|
|
||
| module Ogp | ||
| module Image |
There was a problem hiding this comment.
もう一つのmoduleもですが、基本的にはclassで設計すべきです。
moduleは他のクラスにmix-inするために使うのがメインです。
| @@ -41,6 +43,8 @@ def create | |||
| def update | |||
| set_wip | |||
| if @article.update(article_params) | |||
| Ogp::Image::AttachmentProcessor.fit_to_size(@article.thumbnail) if @article.thumbnail.attached? | |||
There was a problem hiding this comment.
saveの時と全く同じならば共通化した方がミスが少なくなると思います。
There was a problem hiding this comment.
こちら971829fで修正しました！
今回の修正で実際にミスが発生してしまったので痛感しました😅
wata00913
force-pushed
the
feature/fit-to-size-for-ogp-thumbnails
branch
from
d5ec602 to
971829f
Compare
|
@komagata |
app/models/ogp/image_processor.rb
Outdated
| @@ -0,0 +1,54 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| module Ogp | |||
There was a problem hiding this comment.
全てクラスメソッドになっているので、まずクラスをインスタンス変数とインスタンスメソッドで設計してみてください。
全てクラスメソッドなので外部から不要なメソッドも呼び出せるようになっています。
クラス名が表すものとそれが持つ変数（名詞）、それが持つメソッド（動詞）をやりたいことの文章から設計してみるといいかもです。
また、Ogp::ImageProcessorですと、他のOgp画像も同じ縦横で処理するという意味合いになると思うのですが、その辺りも検討したほうがいいかもです。（practiceのogp imageやpageのogp imageなど）
There was a problem hiding this comment.
Ogp::ImageProcessorですと、他のOgp画像も同じ縦横で処理するという意味合いになると思うのですが、その辺りも検討したほうがいいかもです。
こちらのコメントにて、他のOgp画像にも同じ処理を使うとのことなので、意図的に Ogp::ImageProcessorにしました。
There was a problem hiding this comment.
クラスの設計を再検討し、３つの案を考えました。
懸念点を踏まえると「案２」を採用しようと考えてますが、ご指摘でデータ構造や処理自体を大幅に変更する可能性があるため、実装前にクラス設計のレビューをお願いします🙇♂️
クラス図の補足説明
-privateメソッド+publicメソッド
案１
１つのクラスで再構成しました。 new 以外のメソッドはインスタンスメソッドになります。
- 懸念点
fit_to_size_optionをテストしたいが、privateメソッドのテストが必要。fit_to_size_optionの呼び出し元fit_to_sizeは、加工した画像のサイズ(1200px x 630px)のテストは可能だが、適切なオプションで画像加工がされたか判断することが出来ない。
classDiagram
class `Ogp::ImageProcessor`{
WIDTH
HEIGHT
attached_one
+ new(attached_one)
+ fit_to_size()
- just_fit_to_size?()
- fit_to_size_option()
- resize_option(width, height)
- clip_option()
}
案２
案１でprivateメソッドのテストがあるため、クラスの分割をしました。
- Ogp::TransformOption
- OGP画像に加工するオプションを作成するクラス
classDiagram
`Ogp::ImageProcessor` *-- `Ogp::TransformOption`
class `Ogp::TransformOption` {
width
heigth
+ new(width, height)
+ fit_to_size_option(original_width, original_height)
+ just_fit_to_size?(original_width, original_height)
- resize_option(original_width, original_width)
- clip_option()
}
class `Ogp::ImageProcessor`{
attached_one
transform_option
WIDTH
HEIGHT
+ new(attached_one)
+ fit_to_size()
+ just_fit_to_size?()
}
案３
OGP画像の加工に必要なオプション作成のみをクラスにします。
以下の処理はOgp::ImageProcessorではなく、コントローラーで実行します。
-
10行目〜13行目にある元画像の(幅, 高さ)の取得
-
19行目〜23行目にある画像加工（元の画像の削除も含む）処理は、コントローラーで行います。
-
懸念点
- 他ページ(practiceやpage)のOGP画像にも今回の加工処理を適用する予定のため、コントローラーに書く処理が重複してしまう。
initializeの引数にOGP画像のサイズ(1200, 630)を渡す必要があり、(1200, 630)を渡す処理も重複する- また、(1200, 630)を定数としてクラス内で定義した場合、メソッド群をわざわざインスタンスメソッドにする必要性が無くなる。
classDiagram
class `Ogp::TransformOption` {
width
heigth
+ new(width, height)
+ fit_to_size_option(original_width, original_height)
+ just_fit_to_size?(original_width, original_height)
- resize_option(original_width, original_width)
- clip_option()
}
|
@wata00913 返信を忘れており、大変申し訳ありません。 |
|
@komagata
こちら、了解しました。以下コメントで定数の指針が決まり次第、再実装と同時にconflict修正をしようと思います。 前回駒形さんから頂いたレビュー指摘#6483 (comment) にて、OGPサイズの定義箇所を以下のように考えましたが、いかがでしょうか？
理由としては、#6415 (comment) にて、他ページでもOGP画像のリサイズのサイズで利用されるので、Railsの |
|
@komagata @wata00913 現在以下のIssueが進行してます。 画像変換のライブラリが変更となるので、このIssueに影響があるのではとコメントしました。 私も下記Issueを担当しますが、本Issueで画像切り取りのコードが共通化されそうなので、これを待って実装しようかと思っています。 実装の順序、方針などを示しただけると助かります！ |
はい、影響ありますね。
それで良いと思います。 @kyokucho1989 @wata00913 もしかしたら下記で話している内容（複数種類の変換を一度に書ける）で簡単になる部分があるかもなので結果をお待ちいただければと思います。 |
|
@wata00913 こちらで @yocchan-git さんがおっしゃってるやり方でいけますでしょうか。 |
|
@wata00913 さん こちら切り抜きについて、僕の方でクラスを作成して共通化させていただきます！ 経緯はこちら |
|
@wata00913 さん wataさん、お疲れ様です。画像リサイズの処理を共通化しましたのでご確認いただきたく🙏 処理の流れとしましては、この部分でインスタンス化して使用します。詳しく解説します 処理の流れインスタンスを生成する際、 そして、 要約完全に理解する必要はないのですが、要するに
という処理ができます。こう言った処理でよろしかったでしょうか？🤔 また、kyokuchoさんも後ほど実装すると思いますのでccさせていただきました！ |
|
@yocchan-git |
|
@yocchan-git cc: @kyokucho1989 お疲れ様です！共通化ありがとうございます🙇♂️ コメント拝見しました。大まかな処理はあってそうでした。 1つ目の質問です。
ここの基準選定がyocchanさん担当Issueと僕のIssue #6415 で違ってるかもしれませんが、いかがでしょうか？ しかし、実装して頂いたロジックで僕のIssueの要件も満たせるかもしれませんので、動作確認してみます リサイズの詳細は、下記リンクを参考していただけると助かります🙇♂️
2つ目の質問です。
この切り取りは、「画像の真ん中を切り取る」という認識であってますでしょうか？ |
|
@wata00913 さん、質問いただきありがとうございます！
見ました。ありがとうございます！🙏 おっしゃる通り。「変形前のアスペクト比を維持した状態で拡大する。」という点で違いますね🤔 共通化できないか、確認してみます！💪
おっしゃる通り、真ん中を切り取ります😁 |
|
@yocchan-git @wata00913 |
|
@wata00913 さん machidaさん、ありがとうございます！説明します🙇♂️
ですので、正方形の時などに仕様が違います。(こちらのコメントの一節をから例)
僕の実装したコードでは、(w, h) = (500, 500)の時、いきなり(w’, h’) = (1200, 630)というのが主な違いになります🫡 その上で、wataさんに質問なのですが、
という場合の詳細条件を教えていただけますでしょうか？ machidaさんでも構いませんので、お手隙でご回答いただけますと嬉しいです🙏 |
|
@yocchan-git (cc @wata00913) ごめんなさい🙇 アイコンもアスペクト比を維持するようにしていただきたいです。 画像加工のイメージを作成しました。 ＜アイコン画像トリミングのイメージ＞ ＜サムネイル画像トリミングのイメージ＞ |
|
@wata00913 さん 仕様を満たすオプションが見つかりましたので、こちらで構わないか確認いただきたいです。🙇♂️ こちらについて説明すると、ざっくりというなら
という実装になると思います。🙏 ご確認いただけますと嬉しいです🙇♂️ |
|
@yocchan-git cc: @machida
ご提示頂いたvariantメソッドの |
|
@wata00913 さん それでは、共通化するまでもなさそうですので 長いことお話しありがとうございました kyokuchoさん、上記コメントとのことです！よろしくお願いいたします |
|
@yocchan-git
ということですね。 私が取り組む予定のIssue （#7383）は、 ということで、よいでしょうか？ |
|
komagataさんのレビューによりますが、こんな感じで実装できるかなと思います よろしくお願いします🙇 |
|
すみません、ベースブランチを誤って変更してしまいました。 もとに戻せないか確認します |
|
戻せました。お騒がせしました。 @wata00913 |
|
お疲れ様です。返信が遅れてしまいすみません🙇 kyokuchoさんのと私の処理を共通化するかどうかですよね。 @machida システム側で |
|
@wata00913 OGPでしか使うことはないので、加工前の画像は保存しなくて大丈夫ですー |
|
@wata00913 @machida |
Issue
概要
ブログ記事のサムネイル画像をアップロードする際に、自動で1200px x 630pxに加工する機能を追加しました。サイズ
1200px x 630pxはOGP画像に適したサイズとなります。今後、他の画面でもOGP画像用に加工する機能を使うため、モジュール化して共通利用出来るようにしました。
画像の加工方法
画像の加工方法は2つの手順があります。
1200px x 630pxで切り抜くImageMagicの参考文献
画像の加工は、ImageMagicのコマンド・オプションを
ActiveStorage::Attachment#variantの引数に指定することで実現します。コマンド・オプションの検討には以下の記事が参考になりました。
+repage+repageの代替方法その他参考文献
Ruby on Railsで定数の指定 #Ruby - Qiita
変更確認方法
feature/fit-to-size-for-ogp-thumbnailsをローカルに取り込むkomagataでログインする/articles/newにアクセスする/articles/{article_id}にアクセスする1200px x 630pxであることを確認するScreenshot
画像の加工にて拡大操作をする画像
変更前
変更後
画像の加工にて縮小操作をする画像
変更前
変更後
