change google analytics interface #521
Conversation
|
I agree, we should have separate gin module for that |
| } | ||
|
|
||
| private final Class<? extends PlaceManager> placeManagerClass; |
There was a problem hiding this comment.
Good catch, line 64 could be removed a swell
|
Could you document the breaking change in the description of the review? |
|
Added the breaking change info to description. My workspace has partial work on Universal Analytics in it so it's a bit of a pain to commit minor fixes to this branch at the moment. Maybe wait a day or two and I'll submit a pull with UA support. (Or just continue on this branch) This code still needs to be reviewed though. I need to know if the init method can be removed or if the current code loads GA like that for a reason. |
|
The init method add the script to the html page asynchronously. This method isn't perfect. For exemple, there's no way to know when the script will be loaded and it can happen that GA isn't loaded when attempting a track event too soon. |
|
Ok I don't think the init method is blocking because it just sets up an empty array and then adds an async script to the header. Events tracked before the ga.js loads are pushed to the array until everything's ready. It might block on older browsers that don't support async but I don't think that's a major concern. |
|
I don't think as well. |
|
This doesn't seem to be breaking that much. In the sense that most people use the GoogleAnalyticTracker which use init() in turn. You already made the change to that class as well. |
|
Ok, so google analytics only updates it's data after 24 hours so I haven't been able to verify that this works at all. But this patch should make it possible to use universal analytics. I'll see if I can work out how to add a wiki page for it now. |
|
wiki on how to use universal analytics: |
|
Just tested and this doesn't actually work yet. Working on fix |
| if ($wnd.console) { | ||
| $wnd.console.log("uaNative: "+ params); | ||
| } | ||
| $wnd.__ua.apply($wnd.__ua, params); |
There was a problem hiding this comment.
This call to apply is having no effect. Can someone good with Javascript tell me what I've done wrong?
There was a problem hiding this comment.
params will be an array which will log to the console: eg: ["send","pageview",{"page":"/library"}]
In my code when I call $wnd.__ua.apply with null, $wnd or this as the first param I see no network activity.
But when I go to the console on a live page and type window.__ua.apply(null, ["send","pageview",{"page":"/library"}]) it works perfectly.
No idea why the same command doesn't work from jsni.
| bind(GoogleAnalyticsNavigationTracker.class).asEagerSingleton(); | ||
| } | ||
| } | ||
|
|
|
I'm happy with this code and have started using it in production. Let me know if there are outstanding issues preventing merging. |
|
It'll get picked up by the team today or tomorrow. Since it is a big change, I prefer letting it be reviewed thoroughly. Also, we started to extract everything that is relation to Google Analytic in another open source project and we may move this code there. |
|
Well, move is a big word. But more likely remove it in 2.0, deprecate it in the meanwhile and suggest to use the other one. (I do not want to create a release of GWTP when changes are only needed in Google Analytics) |
|
Ok I wasn't aware of the other project. 100% agree that analytics should be separated out of gwtp into its own project. Even with all this code there are large parts of universal analytics API that I've ignored like the ecommerce plugin and tasks. It's going to be a huge effort to get full coverage. |
|
I've moved development on this to a new project: https://github.com/rdwallis/gwt-universal-analytics I generated most of the code directly from the universal analytics api so it should support 100% of the core api. Anyway I recommend using the new project instead of this pull request. So feel free to close this issue without merging. |
|
We also did: https://github.com/ArcBees/universal-analytics Not in PR right now, but used in GAE-Studio already. This library also On Tue, Jul 15, 2014 at 10:21 AM, Richard Wallis notifications@github.com
Christian Goudreau | CEO - Président |
|
We'll be crediting you the contribution and linking your Github project. This library will become official into the Arcbees products eventually. |
|
https://github.com/rdwallis/gwt-universal-analytics now supports server calls via the measurement api. It'll automatically fill out required fields for you, so you use the server with exactly the same interface as on the client, no extra setup needed. Closing this issue, and I'll continue discussion at the arcbees universal-analytics project. |
Edit: wiki on how to use universal analytics:
https://github.com/ArcBees/GWTP/wiki/Universal-Analytics
This is a related to my post at https://groups.google.com/forum/?fromgroups#!topic/gwt-platform/0cfnA9om3DM
It is a breaking change so needs thorough review before being accepted.
I removed the binding from default module since you now have to bind userAgent at the same time. I think it might be good to add a separate analytics gin module so that analaytics, navigation tracker & UA can be installed all at once.
I moved the GAAccount annotation to the google analytics folder because I think it's a mistake to put all annotations in the same package just because they're annotations. GAAccount.java is much more related to GoogleAnalytics.java than it is to DefaultPlace.java
Edit: Breaking change is that the init method is removed, and googleanalytics is no longer bound by default module. Also this code is completely untested. I'm working on a new project which I'm only adding Universal Analytics to, so someone using the old version of analytics needs to test if everything still works.