Skip to content
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

fix multiple og:image meta tags #629

Merged
merged 6 commits into from
May 19, 2017
Merged

fix multiple og:image meta tags #629

merged 6 commits into from
May 19, 2017

Conversation

Nyoho
Copy link
Member

@Nyoho Nyoho commented Apr 3, 2017

contrib の ogp.rb プラグイン使用時に、どうも default_ogp メソッドによって og:image メタタグが2つ生成されてしまうのを修正しようとしています。(1つ目は default_ogp メソッドが <meta content="diary-url/images/ogimage.png" property="og:image"> のようなタグを、そして2つ目に ogp.rb プラグインが本来の og:image タグを生成していて、前者は not found となりfacebookなどでURLを貼ったときに真っ白けになっています。)

元は、@conf.bannerdefined? かどうかをチェックしていたり .nil? したりしているのですが、

  1. まず https://github.com/tdiary/tdiary-core/blob/master/lib%2Ftdiary%2Fplugin%2F00default.rb#L327 は否定なのではないか? (そうでしょうか?)
  2. さらに、@conf.banner'' だったりすることもあり、nil チェックだけではだめなのではないか?

と考えて、ひとまずこのコード変更をしてみています。
ogp.rb プラグインをお使いの方、特にレビューをお願いしたく存じます。

@machu machu temporarily deployed to tdiary-dev-pr-629 April 3, 2017 11:06 Inactive
@tdtds
Copy link
Member

tdtds commented Apr 4, 2017

実はOGPまわりにはノータッチなので、参考程度に。
1、2ともに、おっしゃるとおりに見えますね。なのでこの変更は有効に思えます。

一方、contrib/ogp.rbの実装はすでに00default.rbとかぶっている部分が多くて、住み分けができていないようにも見えます。ちょっと整理した方がよさそうな気がします。具体的には、00default.rbがカバーしていない「fb:~」や「twitter:~」のようなベンダー依存の拡張を定義するためのものにするとかですね。

@Nyoho
Copy link
Member Author

Nyoho commented Apr 6, 2017

@tdtds レビューいただきありがとうございます。
00default.rb と contrib/ogp.rb の住み分けとなると、このような付け焼き刃的なPRではなく、かなりちゃんと考えた設計をし直した方がよさそうですね。
僕なりにも考えてみますが、引き続き、設計に関するアドバイスも、ogp.rb プラグインをお使いの方を含めていただきたく存じます。

@machu
Copy link
Member

machu commented Apr 6, 2017

本体側にogp対応が入った経緯を追えてないですが、機能がかぶらないように整理してみます。

@tdtds
Copy link
Member

tdtds commented Apr 6, 2017

00defaultに入った経緯は #567 では?

@Nyoho
Copy link
Member Author

Nyoho commented Apr 6, 2017

#567 はぐぐっと改良されたところ (そして ogp.rb プラグインとかぶりがぐぐっと出てきたところ?) で、
#413 が最初に 00default に入ったところですかね。

@machu
Copy link
Member

machu commented May 14, 2017

本体側で基本的なogp要素を出力するようにして、ogpプラグインでは付加的な要素を追加するように整理しました。

ogpプラグインの役割

  • ベンダー依存コードの出力 (fb:app_id, fb:admins)
  • よりリッチなimagesとdescriptionの出力

00default.rb側は以下の修正を加えています。

  • ogpプラグインの判定コードを除去
  • ベンダー依存コードの出力コードを除去 (出力したい人はogpプラグインを使う)
  • すべてのcontent値でHTMLエスケープを実施
  • メソッド名をdefault_ogpからogp_tagへ変更

@machu machu changed the title [WIP] fix multiple og:image meta tags fix multiple og:image meta tags May 14, 2017
@tdtds
Copy link
Member

tdtds commented May 19, 2017

もっとガッツリ住み分けてもいいかなーとも思いますが、とりあえずこれでいったんマージしましょう。

@tdtds tdtds merged commit cf7aebe into master May 19, 2017
@tdtds tdtds deleted the fix-multiple-og-image branch May 19, 2017 06:44
@Nyoho
Copy link
Member Author

Nyoho commented May 19, 2017

bannerと本文の画像が両方 og:image になってしまいますねえ。 contribもアップデートするのを忘れていました。後者のみになりました。

@machu
Copy link
Member

machu commented May 21, 2017

問題ないようで良かったです。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants