add support for google analytics userId #15
Conversation
|
looks good from my side. 👍 for merging |
|
please no version bumping in PRs |
|
|
||
| tracker_options | ||
| end | ||
| end |
There was a problem hiding this comment.
could this maybe more generic? always camelize the keys, support #call on every option....
There was a problem hiding this comment.
good point and useful for the cookie_domain with an i18n multihost setup 17000d3
| @@ -93,6 +121,15 @@ def env | |||
| end | |||
| end | |||
|
|
|||
| describe "with user_id tracking" do | |||
There was a problem hiding this comment.
isn't that something that should only be tested in the integration spec?
There was a problem hiding this comment.
the cookieDomain thingy is tested here too.
both can be tested again in the integration spec too.
There was a problem hiding this comment.
ok i'm just a bit confused as to what i should test where
* remove one if * refactor spec
| @@ -28,6 +31,18 @@ def tracker | |||
| options[:tracker].respond_to?(:call) ? options[:tracker].call(env) : options[:tracker] | |||
| end | |||
|
|
|||
| def tracker_options | |||
| @tracker_options ||= begin | |||
| tracker_options = {} | |||
There was a problem hiding this comment.
i know its a matter of taste, but personally i don't like these temporary variables, tap is your fried to get rid of 'em
add support for google analytics userId
|
could we agree on not merging stuff that is under discussion and not reviewed completely? |
|
I will be more careful next time. |
See:
https://developers.google.com/analytics/devguides/collection/analyticsjs/user-id