cmackay / google-analytics-plugin

Cordova Google Analytics Plugin for Android & iOS
Apache License 2.0
89 stars 43 forks source link

GTM first cut #22

Closed zmagyar closed 9 years ago

zmagyar commented 9 years ago

Added interface to

zmagyar commented 9 years ago

@cmackay I have removed the cordova.define call which was added by cordova and I accidentally left there (file copied from my test folder).

Your problems can be down to several reasons:

On this container I can call:

    analitics.getConfigStringValue('testConfig', function(value) {console.log("Container value retrieved " + value);}, function(err) {console.log(err);});
    analitics.getConfigBoolValue('boolValue', function(value) {console.log("Container bool value retrieved " + value);}, function(err) {console.log(err);});
    analitics.getConfigIntValue('intValue', function(value) {console.log("Container int value retrieved " + value);}, function(err) {console.log(err);});
    analitics.getConfigFloatValue('doubleValue', function(value) {console.log("Container double value retrieved " + value);}, function(err) {console.log(err);});

Also I can access macro values defined in the container by default. E.g.

    analitics.getConfigStringValue('id for advertising', function(value) {console.log("Container value retrieved " + value);}, function(err) {console.log(err);});

I hope this helps to get it working.

BTW are you ok with the API function names and with the way how the API is built up so far? I'm happy to adjust if you have better way to do it.

cmackay commented 9 years ago

Great! Thanks. @zmagyar

As for the API, I think what you have defined looks like a good approach. I have been thinking about some ideas and I think how you have it is a good way to go. Some other possible ideas are we could split the gtm functionality into new script or namespace and possibly refactor the overall structure of the plugin. One thought I was thinking was to have a namespace under which both ga and gtm functionality live.

Currently the plugin is accessible under navigator.analytics but it could maybe be navigator.ga.tracker and navigator.gtm.container then the function names could be be more specific since they would not potentially conflict.

other options: (or below excluding gms) gms.analytics.tracker gms.tagmanager.container

Just some thoughts. What do you think about something like that?

cmackay commented 9 years ago

Also I was able to get the config values from the container. :) Thanks! I am going to start implementing the android methods for gtm

zmagyar commented 9 years ago

Hi Craig,

My initial idea was to create a separate plugin for GTM. But most likely GA and GTM used together anyway so it makes sense to keep them in a single plugin. However I think it would make perfect sense to separate the namespaces. E.g. we need to introduce a function to set the log level for GTM. The naming of that function would be more consistent if a separate namespace is used.

The refactoring of the existing API requires some consideration IMO as it would affect existing plugin users. Therefore if you plan to refactor the API then I would prefer the option where the existing API would remain untouched. However if you see any advantage to move that direction then lets do it.

Z

zmagyar commented 9 years ago

Glad you got it working now. :)

Hopefully I will have some more spare time soon to move the iOS version forward. I will check how far you get with the droid version and will adjust the existing iOS code. Is it ok to continue pushing my changes here and do pull request to the gym branch?

Z

cmackay commented 9 years ago

Sounds great. Yeah feel free to submit pull requests as you have changes. Thanks!

Yeah, I don't want to disrupt any current users but the plugins are versioned so I think it is fairly safe to make changes to the API. We can release this as a new minor version 0.2.x, since it is a significant change :) I thinking for the new API we could have something like:

analytics.tracker.Fields; analytics.tracker.HitTypes; analytics.tracker.LogLevel;

analytics.tracker.open(trackerId, success, error); analytics.tracker.setLogLevel(logLevel, success, error); analytics.tracker.enableAdvertisingIdCollection(success, error); analytics.tracker.sendAppView(screenName, success, error); analytics.tracker.sendAppViewWithParams(screenName, params, success, error); analytics.tracker.sendEvent(category, action, label, value, success, error); analytics.tracker.sendEventWithParams(category, action, label, value, params, success, error); analytics.tracker.sendException(description, fatal, success, error); analytics.tracker.trackUnhandledScriptErrors(opts, success, error); analytics.tracker.customDimension(id, value, success, error); analytics.tracker.customMetric(id, value, success, error); analytics.tracker.set(name, value, success, error); analytics.tracker.get(name, success, error); analytics.tracker.send(params, success, error); analytics.tracker.close(success, error);

tagmanager.container.open(containerId, success, error); tagmanager.container.setLogLevel(logLevel, success, error); tagmanager.container.refresh(success, error); tagmanager.container.getString(key, success, error); tagmanager.container.getBoolean(key, success, error); tagmanager.container.getDouble(key, success, error); tagmanager.container.getLong(key, success, error);

what do you think?

zmagyar commented 9 years ago

Looks good to me. There is only one comment. I think the LogLevel constants might be the same for GTM and GA so I'm not sure that needs to be defined on analytics.

Z

cmackay commented 9 years ago

sounds good. Hopefully there is some reuse between the two. I have noticed some inconsistencies in the past with the log level constants between iOS and Android for the analytics logger. (pretty odd)

kGAILogLevelNone = 0, kGAILogLevelError = 1, kGAILogLevelWarning = 2, kGAILogLevelInfo = 3, kGAILogLevelVerbose = 4

public static final int VERBOSE = 0 public static final int INFO = 1 public static final int WARNING = 2 public static final int ERROR = 3

zmagyar commented 9 years ago

To make it more complicated tag manager just defines

kTAGLoggerLogLevelVerbose, kTAGLoggerLogLevelDebug, kTAGLoggerLogLevelInfo, kTAGLoggerLogLevelWarning, kTAGLoggerLogLevelError, kTAGLoggerLogLevelNone

So it is supposed to be

kTAGLoggerLogLevelVerbose = 0 kTAGLoggerLogLevelDebug = 1 kTAGLoggerLogLevelInfo = 2 kTAGLoggerLogLevelWarning = 3 kTAGLoggerLogLevelError = 4 kTAGLoggerLogLevelNone = 5

Therefore It might make sense to define our own mapping to create consistent API. I.e. define the plugins own log level contacts and then map it to the google SDK log levels (even with some overlap).

cmackay commented 9 years ago

I think that sounds like a great idea. Too many inconsistencies

cmackay commented 9 years ago

I checked in the android changes to match the current interface. I am happy to work on the api changes we discussed unless you were wanting to. Either way works for me :)

cmackay commented 9 years ago

@zmagyar I was thinking it might be good if you focus on implementing as many functions that you think may be useful from the iOS sdk and then I will get the api changes made and working on the Android changes. You can name the functions whatever seems appropriate and then as we build out the javascript api changes we can see what works best. I really appreciate your contributions with this :) Let me know if this sounds good or any ideas you might have for it. I think the data layer features might also be interesting. I am still looking over many of the apis. It looks pretty cool.

Thanks!

-Craig

zmagyar commented 9 years ago

Hi Craig,

I think I will move the iOS bit forward as time permits. I will continue to use the existing API and when most of them are ready then we can discuss what would be the most logical API.

Z