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

amp.rb: extend callback for supporting Google Analytics #610

Merged
merged 7 commits into from Mar 2, 2017

Conversation

machu
Copy link
Member

@machu machu commented Jan 2, 2017

ampプラグインにてGoogle Analyticsをサポートしました。

@machu machu temporarily deployed to tdiary-dev-pr-610 January 2, 2017 21:50 Inactive
@tdtds
Copy link
Member

tdtds commented Jan 4, 2017

これって、通常のAnalyticsのTracking IDとはぜんぜん別なんです?

@machu
Copy link
Member Author

machu commented Jan 4, 2017

同じでもOKです。別に設定するのを推奨だったので、分けて指定できるようにしています。
https://developers.google.com/analytics/devguides/collection/amp-analytics/?hl=ja

@tdtds
Copy link
Member

tdtds commented Jan 4, 2017

なるほど。しかし、もし別IDにするとしても、Analyticsを使ってる人はcontribのgoogle_analytics.rbも入れているはずなので、両IDを別々のところで指定するのは好ましくないように思います。

  • google_analytics.rbで共通の、ないし別々のIDを指定できるようにする
  • amp.rbではその値があれば使う

というあたりが良さそうに思えますが、どうでしょう。

@machu
Copy link
Member Author

machu commented Jan 4, 2017 via email

@tdtds
Copy link
Member

tdtds commented Jan 4, 2017

依存というほどのものではない(コードには依存しない・設定キーだけ共有している)ので、まぁいいんじゃないですかね。
厳密なことを言えば、特定ベンダー/サービスに関するコードはcontribに出したほうがいいので、本来ならgoogle_analytics.rbの中でAMPページへのコード埋め込みをすべきですが、そこまで複雑にしなくてもな、とは思います。

@machu
Copy link
Member Author

machu commented Jan 4, 2017 via email

@tdtds
Copy link
Member

tdtds commented Mar 2, 2017

(squashで)マージしちゃって良いですよ。

@machu machu changed the title amp.rb: support Google Analytics amp.rb: extend callback for supporting Google Analytics Mar 2, 2017
@machu
Copy link
Member Author

machu commented Mar 2, 2017

悩んだ末に、google_analytics.rb側で機能を拡張できるようにしました。このPRでは拡張用のcallbackを追加します。

@machu machu merged commit e898cc6 into master Mar 2, 2017
@tdtds
Copy link
Member

tdtds commented Mar 2, 2017

なんか大仰な感じになっててちょっとびっくりしたw

@machu
Copy link
Member Author

machu commented Mar 2, 2017

最初はベタに書いていたのですが、どうにもしっくりこなくて、こうなっちゃいましたw

@machu
Copy link
Member Author

machu commented Mar 2, 2017

google analytics用のコードはcontrib側で対応した。
see tdiary/tdiary-contrib@b3a97d6

@tdtds tdtds deleted the feat/amp-analytics branch March 6, 2017 01:19
@tdtds tdtds moved this from 作業中 to 完了 in 5.0.4リリース Mar 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants