search

orta / ARAnalytics

Simplify your iOS/Mac analytics
MIT License
1.84k stars 217 forks source link

Dynamic page names in DSL for screen tracking #246

Closed arifken closed 8 years ago

arifken commented 8 years ago

In our analytics implementation, the page name for screen views can only be derived at the time when the screen view is triggered. This PR allows the developer to define the ARAnalyticsPageName value in the ARAnalyticsProperties dictionary that is returned from the block that is executed when a screen view is triggered.

With this PR, the code checks for the screen view page name in 3 places (in order)

  1. ARAnalyticsPageName (set at configuration time in the ARAnalyticsDetails dictionary)
  2. ARAnalyticsPageNameKeyPath (set at configuration time in the ARAnalyticsDetails dictionary)
  3. ARAnalyticsPageName (set at the time the event is triggered, in the dictionary returned from ARAnalyticsProperties value block)
orta commented 8 years ago

@ashfurrow - got opinions on this?

ashfurrow commented 8 years ago

Hmm, interesting. So from the three screen page name options listed, ARAnalytics already supports 1 and 2. So we should focus on 3.

The goal seems to be to be able to dynamically generate a screen name at runtime, rather than configuration time. That makes sense, but that's what the keypath (option 2) is for, to let developers specify their own keypath that ARAnalytics will use to retrieve a (possibly dynamic) screen name. I'm onboard with providing easier ways to do this, but I'm not sure this solution aligns well with the existing behaviour.

Specifically, we currently pass through the properties dictionary through to the analytics provider, but with this PR, we've put the screen name in there too, and we need to then remove it. I'm not sure that the option of adding a screen name to the dictionary is outweighed by the cognitive load of having to remember that the dictionary gets passed straight through except the screen name. I can see developers opening issues about this :sweat_smile:

So the idea is sound, but I'm not sure about the implementation. Maybe an additional key/value pair with a block to specify the screen title at runtime, instead? It's more explicit and keeps the properties dictionary simple. Thoughts @arifken?

arifken commented 8 years ago

@ashfurrow good point! I I like that much better. so something like ARAnalyticsPageNameBlock that would go under the ARAnaltyicsDetails dictionary, with a value being a block that passes the same arguments as the ARAnalyticsProperties block, but returns an NSString?

arifken commented 8 years ago

If that sounds good I can make the change today and update the PR

ashfurrow commented 8 years ago

Yup, sounds good to me 👍 If you can, adding a changelog entry and updating the Readme would be stellar :telescope:

arifken commented 8 years ago

perfect will do

arifken commented 8 years ago

@ashfurrow I closed this PR and opened a new one for defining both screen and event names dynamically: https://github.com/orta/ARAnalytics/pull/249. When you get a chance, let me know if you think we're on the right track architecture-wise. thanks!