-
Notifications
You must be signed in to change notification settings - Fork 293
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
Simplify Analytics UX for existing tags and GTM properties #4703
Comments
@aaemnnosttv IB almost ✅ , but similar to #4702 let's add a note on Storybook adjustments. |
@eclarke1 @FlicHollis @aaemnnosttv Just leaving a note here that whenever this goes into a sprint/release, we should make sure it eventually lands in the same release as #4702, to not cause some weird "intermediate" user experience. |
IB ✅ |
Hi @aaemnnosttv, I have a question about the requirement to merge At present, the messaging for a pre-existing UA Analytics tag vs a pre-existing GTM tag is mutually exclusive, with the UA tag taking precedence. For example: site-kit-wp/assets/js/modules/analytics/components/settings/SettingsForm.js Lines 70 to 71 in 5f32888
Do we want to retain this logic when merging the two components? Or, do we want to add extra GTM-related conditions to all of the existing notices in It would also raise questions about the structure of the text. For example - If there is an existing Analytics tag and GTM tag, and both match the selected property ID, do we need to mention them both in the text or can we abbreviate it by only mentioning one of them? We'd potentially end up with sentences like this, and all the various permutations it suggests...
cc @felixarntz |
Good question @techanvil – I'm not entirely sure as the IB here is actually from @felixarntz 's original proposal. My guess is that the logic and wording would be mostly the same but merged into the same component. @felixarntz is there any more nuance to that part of the AC or IB that could be clarified? |
@techanvil @aaemnnosttv We should merge the two notice components in a way that maintains the current logic, i.e. any GTM property notice should only show if no existing tag notice applies already. So the merging is mostly to centralize that logic, rather than having to have a conditional display of two components everywhere we need it. |
Cool, thanks for clarifying that @felixarntz 👍 |
@aaemnnosttv @felixarntz in the SettingsEdit component we have a check that forces to display the site-kit-wp/assets/js/modules/analytics/components/settings/SettingsEdit.js Lines 87 to 92 in 398eaa0
|
Hi @eugene-manuilov, I did wonder that myself, but then I noticed that |
QA Update:
|
Hi @wpdarren, I think I see what's happening here. When there is both an existing GTM tag and an existing UA tag that have been inserted manually, the messaging for the UA tag takes precedence over the GTM message, and so the GTM tag is not mentioned (see earlier comments on this issue). Therefore to reproduce the Hopefully that will resolve this for you, but please let me know if not. Cheers |
QA Update: ❌@techanvil I'm sending this back to Execution because I am unable to recreate the Here are the steps I took:
As you can see only the GA4 property is coming up in the message. In the QAB it suggests that the tag manager. Side note: I also created a UA property manually, but didn't add the code to the header.php and tag manager still wasn't referenced. |
Hi @wpdarren, this is a bit of a mystery, as I've just tried this out again myself, on a new site and using the zip you linked to, and was able to view the expected message. I've not done anything outside of the steps you've described. Perhaps it's best if we jump on a call to try to figure this out? |
QA Update: ✅Verified: Existing Analytics tag:
Existing GTM property:
Existing GTM property & GA4 tag:
|
Feature Description
Building on the work started in #4702, this issue is about simplifying the remaining experience for existing tags and GTM properties.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
E.g. “Please select the account information below…” is only shown when there is no existing tag because inputs are disabled in this case. Now these inputs will always be user-editable regardless if there is an existing tag or not.
The PR for this issue should target the
feature/existing-tag-simplification
branchImplementation Brief
For the Analytics module only
hasExistingTag
andgetSingleAnalyticsPropertyID
fromAccountSelect
andPropertySelect
, so that these never show as disabledExistingGTMPropertyNotice
intoExistingTagNotice
, combining the twoExistingTagNotice
, i.e. state that there is an existing GTM property and, if applicable, that it is the same one as the one selected here which is why then Site Kit won’t place the tagExistingGTMPropertyNotice
and anywhere that it’s renderedStorybook
Test Coverage
QA Brief
Existing Analytics tag:
header.php
file.Existing GTM property:
header.php
file.Existing GTM property, matches selected ID:
Existing GTM property, does not match selected ID:
Existing GTM property & GA4 tag:
header.php
file.header.php
file.Existing GTM property & GA4 tag, both match selected IDs:
Existing GTM property & GA4 tag, property matches selected ID, tag doesn't:
Existing GTM property & GA4 tag, tag matches selected ID, property doesn't:
Existing GTM property & GA4 tag, do not match selected IDs:
Changelog entry
The text was updated successfully, but these errors were encountered: