adamberenzweig
commented
7 years ago meta: Faster turnaround on code reviews would make life easier for me. I've merged master into this branch 3 or 4 times since sending the review, with conflicts to deal with each time.
On Mon, Mar 13, 2017 at 4:00 PM, Rich Burdon notifications@github.com wrote:
@richburdon commented on this pull request.
In sub/app/src/client/common/analytics.js https://github.com/minderlabs/demo/pull/69#discussion_r105754193:
- Most services support a similar interface for custom events, Google Analytics seems to be the exception:
- Segment: event(name, params) // https://segment.com/docs/sources/website/analytics.js/#track
- FirebaseAnalytics: event(name, params) // https://support.google.com/firebase/topic/6317484?hl=en&ref_topic=6386699
- Mixpanel: event(name, params)
- Intercom: event(name, params) // https://developers.intercom.com/reference#events
- GoogleAnalytics: event(category, action, label, numeric_value)
- *
- */
- track(name, params) {
- throw new Error('Not implemented.');
- }
- // TODO(madadam): Add a group() method? for e.g. https://segment.com/docs/sources/website/analytics.js/#group +}
+export class SegmentAnalytics extends Analytics {
comment; e.g., add any important links (dashboard?) here). same for class below.
In sub/app/src/client/common/analytics.js https://github.com/minderlabs/demo/pull/69#discussion_r105754242:
- */
- track(name, params) {
- throw new Error('Not implemented.');
- }
- // TODO(madadam): Add a group() method? for e.g. https://segment.com/docs/sources/website/analytics.js/#group +}
+export class SegmentAnalytics extends Analytics {
- constructor(config) {
- super(config);
- // TODO(madadam): will this work in CRX? Might need to use the node library.
- this.analytics = function(){
sp
In sub/app/src/client/common/base_app.js https://github.com/minderlabs/demo/pull/69#discussion_r105754665:
@@ -39,6 +40,8 @@ export class BaseApp { // Manages Apollo query subscriptions. this._queryRegistry = new QueryRegistry();
- this._analytics = new GoogleAnalytics(this._config);
where is this._config coming from (cut-and-paste error?)
In sub/app/src/client/common/reducers.js https://github.com/minderlabs/demo/pull/69#discussion_r105755424:
@@ -152,6 +154,9 @@ export const AppReducer = (injector, config, registration=undefined) => {
// TODO(burdon): Get search query (not just text). case AppAction.ACTION.SEARCH: {
- // TODO(madadam): Add delay or only log final query -- now we send an event for every keystroke, it's overkill.
Simpler to track all redux actions? There's already a delay in TextBox (100ms); you're not typing fast enough.
In sub/app/src/client/common/analytics.js https://github.com/minderlabs/demo/pull/69#discussion_r105755643:
- // TODO(madadam): More events.
- // x Login/logout
- // x Page view / navigation events
- // x Search
- // x Mutations (add|edit, type)
- // - Search result clicks
- // - High-level product actions, e.g. Assign a task, complete a task, add search result to card.
- // - UI actions, e.g. drag a card, button clicks.
- // - Clicks on external links (e.g. ReactGA.outboundLink or OutboundLink component).
- // - Exceptions/errors
- constructor(config) {
- this._config = config;
- }
- identify(userId) {
why are there different methods rather than tracking events by type?
In sub/app/src/common/defs.js https://github.com/minderlabs/demo/pull/69#discussion_r105756075:
@@ -39,6 +39,17 @@ export const FirebaseTestingAppConfig = { };
/**
- Analytics
Links above vars to provenance of keys (I also put dates in [] for keys/artifacts that are created externally).
In sub/core/src/data/mutations.js https://github.com/minderlabs/demo/pull/69#discussion_r105756506:
//
- mutator: new Mutator(ownProps.idGenerator, mutate)
- mutator: new Mutator(ownProps.idGenerator, ownProps.analytics, mutate)
let { idGenerator, analytics } = ownProps
In sub/core/src/data/mutations.js https://github.com/minderlabs/demo/pull/69#discussion_r105756810:
@@ -324,6 +325,8 @@ export class Mutator { } });
- this._analytics && this._analytics.track('Create Item', {label: type});
constants for all labels co-located in defs? Also, 'item.create' ('item.foo', etc.) instead?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/minderlabs/demo/pull/69#pullrequestreview-26648679, or mute the thread https://github.com/notifications/unsubscribe-auth/AAWAUfXlqVwqSwtXScmNp4RLM4NLjqviks5rlaBBgaJpZM4MWE5z .
richburdon
GA dashboard: https://analytics.google.com/analytics/web/#realtime/rt-overview/a82404502w137842039p142136815/%3Fmetric.type%3D5/
I'm not convinced Google Analytics is the right choice, we can continue to play with it. (and it's free.)