Support for Criteo #17
Conversation
|
|
||
| self.position = :body | ||
|
|
||
| def tracker_options |
There was a problem hiding this comment.
since criteo supports setting those as events, they can be then triggered as events and we don't have to jump through hoops like we did for GA in #15
There was a problem hiding this comment.
I would be -1 for users settings this manually for each criteo request. It should be set once in an initializer. But maybe I didn't fully understand the suggestion.
There was a problem hiding this comment.
agreed, but then look into the last changes in #15 to make this more generic. I see a chance here of making them into Event objects and prepend them into the events array.
There was a problem hiding this comment.
ok. that sounds like a good idea.
* use only one loop in the .erb template * add global events (setSiteType, setAccount, setCustomerId) as Rack::Tracker::Criteo::Events
| end | ||
|
|
||
| describe '#tracker_events' do | ||
| subject { described_class.new(env, { set_account: '1234', set_site_type: ->(env){ 'd' }, set_customer_id: ->(env){ nil } }) } |
There was a problem hiding this comment.
I would add another spec where the parameters are not Procs
There was a problem hiding this comment.
I added a couple more specs in d37c486
|
looks good |
|
please remove pry-debug as this is not a runtime dependency |
|
can this PR be merged? |
|
please add it to the readme, then lets merge it |
|
@jhilden ping? :) |
|
updated the README. i think this can be merged now. |
This PR also already includes the changes from #16 After that one is merged the changes should be more readable.