[11722] Add GA event tracking for the Analytics setup screen. #11776
Conversation
ankitrox
changed the title
|
Storybook is ready:
|
|
Build files for 5087bf3 have been deleted. |
|
Size Change: +132 B (+0.01%) Total Size: 2.19 MB ℹ️ View Unchanged
|
abdelmalekkkkk
left a comment
abdelmalekkkkk
left a comment
There was a problem hiding this comment.
Thanks @ankitrox! This looks great. I left a few minor comments/questions. Please let me know your thoughts.
| await trackEvent( 'moduleSetup', 'cancel_module_setup', moduleSlug ); | ||
| }, [ moduleSlug ] ); | ||
|
|
||
| const isInitialSetupFlow = |
There was a problem hiding this comment.
Can we move this to the top of the component? This works perfectly fine but would sit nicely below the showProgress and the other declarations.
| 'complete_module_setup', | ||
| moduleSlug | ||
| ); | ||
| const completionEventArgs = isInitialSetupFlow |
There was a problem hiding this comment.
Can we use a simple if/else here? It would be more readable in my opinion.
There was a problem hiding this comment.
Thanks for the suggestion @abdelmalekkkkk.
I think we should stick to ternary operator here because of following reasons:
- This is fairly simple expression.
- If we use
if...elsestatement we may have to either calltrackEventtwice or need to uselet completionEventArgswhich is susceptible to change which is a source of potential side-effect bugs.
There was a problem hiding this comment.
Hey @ankitrox, thanks for explaining your rationale here, however, I would agree with @abdelmalekkkkk that it would be more readable to use an if/else. It won't result in calling trackEvent() twice, simply writing it twice, which seems fine when considering the improved legibility:
if ( isInitialSetupFlow ) {
await trackEvent(
`${ viewContext }_setup`,
'setup_flow_v3_complete_analytics_step'
);
} else {
await trackEvent(
'moduleSetup',
'complete_module_setup',
moduleSlug
);
}| 'proxy' | ||
| ); | ||
|
|
||
| const createAccountEventArgs = isInitialSetupFlow |
There was a problem hiding this comment.
My reply above also applies here, please do update this to use an if/else.
| hasGTMScope, | ||
| setValues, | ||
| viewContext, | ||
| isInitialSetupFlow, |
There was a problem hiding this comment.
Any reason why you added isInitialSetupFlow and viewContext to the dependency array here but not in ModuleSetup.js? Doesn't seem like a big deal, but I'm wondering if we can keep things consistent.
There was a problem hiding this comment.
That's a good catch @abdelmalekkkkk , thanks for spotting it.
I've added it now.
| cleanup(); | ||
| } ); | ||
|
|
||
| it( 'should track initial setup flow create account event when showProgress=true', async () => { |
There was a problem hiding this comment.
I think the label needs to be something along the lines of "during initial setup flow" instead of "when showProgress=true", because showProgress is not the only condition.
| expect( genericCreateAccountCall ).toBeUndefined(); | ||
| } ); | ||
|
|
||
| it( 'should track generic create account event when showProgress is not true', async () => { |
|
Thanks for reviewing the PR @abdelmalekkkkk I've addressed the necessary changes and responded on couple of comments. Please let me know if you observe any further issues. Thanks! |
|
Thanks @ankitrox! LGTM ✅ |
| 'complete_module_setup', | ||
| moduleSlug | ||
| ); | ||
| const completionEventArgs = isInitialSetupFlow |
There was a problem hiding this comment.
Hey @ankitrox, thanks for explaining your rationale here, however, I would agree with @abdelmalekkkkk that it would be more readable to use an if/else. It won't result in calling trackEvent() twice, simply writing it twice, which seems fine when considering the improved legibility:
if ( isInitialSetupFlow ) {
await trackEvent(
`${ viewContext }_setup`,
'setup_flow_v3_complete_analytics_step'
);
} else {
await trackEvent(
'moduleSetup',
'complete_module_setup',
moduleSlug
);
}| 'proxy' | ||
| ); | ||
|
|
||
| const createAccountEventArgs = isInitialSetupFlow |
There was a problem hiding this comment.
My reply above also applies here, please do update this to use an if/else.
assets/js/modules/analytics-4/components/common/AccountCreate/index.test.js
Outdated
Show resolved
Hide resolved
assets/js/modules/analytics-4/components/common/AccountCreate/index.test.js
Outdated
Show resolved
Hide resolved
assets/js/modules/analytics-4/components/common/AccountCreate/index.test.js
Outdated
Show resolved
Hide resolved
assets/js/modules/analytics-4/components/common/AccountCreate/index.test.js
Outdated
Show resolved
Hide resolved
|
@techanvil Thanks for reviewing the PR and providing the valuable feedback. I've addressed the changes you suggested and sending your way for another pass. |
assets/js/modules/analytics-4/components/common/AccountCreate/index.test.js
Show resolved
Hide resolved
| let registry; | ||
|
|
||
| beforeEach( () => { | ||
| mockTrackEvent.mockClear(); |
There was a problem hiding this comment.
As per my other comment, please move this line to an afterEach() callback.
3 participants
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist