-
Notifications
You must be signed in to change notification settings - Fork 277
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhance/#6738 - Create new SetupFormGA4
component
#6808
Conversation
Size Change: +1.5 kB (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. A few questions and a few changes needed I think, but overall looks great. 👍🏻
const onChange = useCallback( () => { | ||
setValues( FORM_SETUP, { enableUA: ! isUAEnabled } ); | ||
|
||
trackEvent( `${ viewContext }_analytics`, 'enable_ua' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this event mentioned anywhere in the issue's ACs or IB, and it isn't in the list of Custom Analytics Events for Site Kit either. Was this discussion elsewhere (in Slack or something)? I just want to make sure we have the okay from @marrrmarrr to add this event to the plugin 🙂
trackEvent( `${ viewContext }_analytics`, 'enable_ua' ); | |
trackEvent( `${ viewContext }_analytics`, 'enable_ua' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. It's not mentioned anywhere. I added it. I will remove it. We can add it later if we need an event 👍
checked={ isUAEnabled } | ||
onClick={ onChange } | ||
hideLabel={ false } | ||
disabled={ false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we explicitly need to set disabled
to false
? That seems a bit odd is all—I don't see that anywhere else in the codebase 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Removed it 👍
@@ -1,7 +1,7 @@ | |||
/** | |||
* Analytics GA4 Setup form. | |||
* | |||
* Site Kit by Google, Copyright 2021 Google LLC | |||
* Site Kit by Google, Copyright 2023 Google LLC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Site Kit by Google, Copyright 2023 Google LLC | |
* Site Kit by Google, Copyright 2021 Google LLC |
This file is from 2021, let's leave this alone. The copyright year for the file should be when it was first created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
@@ -1,7 +1,7 @@ | |||
/** | |||
* SetupFormGA4 component stories. | |||
* | |||
* Site Kit by Google, Copyright 2021 Google LLC | |||
* Site Kit by Google, Copyright 2023 Google LLC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Site Kit by Google, Copyright 2023 Google LLC | |
* Site Kit by Google, Copyright 2021 Google LLC |
Same here, let's leave this as-it-was.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
@@ -0,0 +1,131 @@ | |||
/** | |||
* Analytics GA4 Setup form Legacy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Analytics GA4 Setup form Legacy. | |
* Analytics GA4 Setup form (legacy version, supports UA setup). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
// If `ga4Reporting` AND `enableUA` are both enabled, we need to reset the | ||
// property and profile IDs to ensure that the UA settings are not saved. | ||
if ( ga4ReportingEnabled && ! isUAEnabled ) { | ||
dispatch( MODULES_ANALYTICS ).resetPropertyAndProfileIDs(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code comment here mentions GA4 and "Enable UA" both being enabled, but the code here is checking for GA4 Reporting being enabled and UA being disabled. 🤔
I think the code logic is right, just the comment needs to be updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. Updated it.
// Do not require selecting a property or profile if `ga4Reporting` is enabled. | ||
// Therefore, only validate these if `ga4Reporting` is not enabled. | ||
if ( ! isFeatureEnabled( 'ga4Reporting' ) ) { | ||
// Therefore, only validate these if `ga4Reporting` is not enabled OR `enableUA` is enabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Therefore, only validate these if `ga4Reporting` is not enabled OR `enableUA` is enabled. | |
// Only validate UA settings if `ga4Reporting` is not enabled OR `enableUA` is enabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
Summary
Addresses issue:
SetupFormGA4
component #6738Relevant technical choices
SetupFormGA4
component as it requires more effort. In addition, writing basic tests is not possible without providing proper mocks for theanalytics
andanalytics-4
modules.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