Allow for dynamic Facebook Pixel options #101
Conversation
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
a similar method is in https://github.com/railslove/rack-tracker/pull/97/files#diff-8c96bad81146eddcac1965e5645e68f3R11 #97
I think it would be awesome to move it to the Rack::Tracker::Handler class that all the trackers can access it.
What do you think? could you do that?
There was a problem hiding this comment.
Sure, I'll give it a shot!
There was a problem hiding this comment.
yay, that would be a great addition.
* Require `ALLOWED_TRACKER_OPTIONS` to be defined on all classes that use it * Extract key, and value transformations into separate methods that can be overriden in descendants
lib/rack/tracker/handler.rb
Outdated
| # Transformations to be applied to tracker option values. | ||
| # Override in descendants, if necessary. | ||
| def tracker_option_value(value) | ||
| value.to_s |
There was a problem hiding this comment.
why do we need the to_s?
If we use it in the template the <%= %> does a to_s anyway, isn't it?
There was a problem hiding this comment.
That's what the Google Analytics tracker is currently doing. It can potentially make a difference in that case, because the values aren't outputted directly using the <%= %> there, but rather the entire hash is converted to JSON, so it's a difference between "{\"one\":1}" and "{\"one\":\"1\"}". That said, it probably shouldn't be the default behaviour, I can move it to the GA tracker.
There was a problem hiding this comment.
ok, I see. good point.
I guess the convention could be that call should return a string. but I am fine either way.
If that's the case right now in the analytics tracker then let's do it like that.
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
looks good!! should also add some specs for that on the handler level.
There was a problem hiding this comment.
Yep, I'll try to come up with some tests for this. Thanks for the feedback!
Put it in the Google Analytics tracker.
Base them off of the Google Analytics specs.
|
@bumi Would you like me to make some more changes? |
|
I personally don't like the use of a constant in the facebook handler. Since constants are hard to mock etc... I would prefer something like a hook method on the top handler class. But maybe this is something we can refactor later... and go with this solution for now... |
|
nice! looks great! thanks again for the contribution. @DonSchado I actually agree. also it would then be similar to the |
|
should actually be simple to replace, and I think it is more consistent then. |
* Define the :allowed_tracker_options attribute on the Handler class, and set it to an empty array by default. * Use the new class attribute in #tracker_options. * Remove allowed option stubbing from Google Analytics specs. Use a real option instead. * Redo the #tracker_options specs.
|
The constant is dead. ;-) |
|
awesome! let's merge! @DonSchado you're last chance to comment. |
|
💚 |
|
Finally this is included in the last release |
|
Sweet, thanks! |
This allows the Facebook Pixel id (as well as other tracker options) to be dynamic, and change on a per-request basis. It's based on some other trackers that offer this functionality (
:user_idon the Google Analytics one, for example).