Skip to content
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

Change Analytics setup description #5046

Closed
felixarntz opened this issue Apr 6, 2022 · 13 comments
Closed

Change Analytics setup description #5046

felixarntz opened this issue Apr 6, 2022 · 13 comments
Labels
End of Sprint Issues which are due by the End of Sprint Module: Analytics Google Analytics module related issues P0 High priority Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature

Comments

@felixarntz
Copy link
Member

felixarntz commented Apr 6, 2022

Related to #4913 and #5045, some of the descriptive text in the Analytics setup flow should be simplified as it is now replaced with the new toggles displayed there.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Relying on the feature/existing-tag-simplification branch:

  • The Analytics ExistingTagNotice should be simplified to only keep the information around GTM tags, but no longer inform about "existing" tags, as these are covered via Implement Analytics snippet toggle(s) in setup flow if there is an existing tag #4913 and Show Analytics useSnippet toggle when creating a new UA/GA4 property #5045 now.
    • In other words, the only check that should remain in the component is hasGTMAnalyticsProperty, and then it should display the notice from getNoticeForExistingGTMProperty().
  • Throughout all scenarios of the Analytics setup flow, the following minor copy adjustment should be made:
    • Please select the account information below. You can change this view later in your settings. --> Please select the account information below. You can change this later in your settings.

Implementation Brief

Any PR for this must be based on and target the feature/existing-tag-simplification branch.

  • Using assets/js/modules/analytics/components/common/ExistingTagNotice.js,
    • Keep only the check for hasGTMAnalyticsProperty and all the logic associated to it. This means removing the following:
      • getNoticeForExistingUAAndGA4Tags
      • getNoticeForExistingUATag
      • getNoticeForExistingGA4Tag
  • Using the files below, update the text as per the second bullet point of the AC.
    • assets/js/modules/analytics/components/setup/SetupFormGA4.js
    • assets/js/modules/analytics/components/setup/SetupFormGA4Transitional.js
    • assets/js/modules/analytics/components/setup/SetupFormLegacy.js
    • assets/js/modules/analytics/components/setup/SetupFormUA.js

Test Coverage

  • No new tests to be added.

QA Brief

  • GTM Notices will be shown in the top of the Setup and Settings page when relevant.
  • Existing Tag Notice for UA and GA4 will be shown on their respective toggles in Setup page.
  • Existing Tag Notice will continue to surface on top of Settings View and Settings Edit page.
  • The Updated Copy from the AC will be shown on all relevant places.

Changelog entry

  • N/A
@felixarntz felixarntz added P0 High priority Type: Enhancement Improvement of an existing feature Module: Analytics Google Analytics module related issues labels Apr 6, 2022
@felixarntz felixarntz assigned felixarntz and unassigned felixarntz Apr 6, 2022
@asvinb asvinb assigned asvinb and unassigned asvinb Apr 7, 2022
@tofumatt tofumatt self-assigned this Apr 7, 2022
@tofumatt
Copy link
Collaborator

tofumatt commented Apr 7, 2022

IB ✅

@tofumatt tofumatt removed their assignment Apr 7, 2022
@kuasha420 kuasha420 self-assigned this Apr 11, 2022
@kuasha420 kuasha420 removed their assignment Apr 11, 2022
@asvinb asvinb assigned asvinb and kuasha420 and unassigned asvinb Apr 11, 2022
@kuasha420 kuasha420 assigned asvinb and unassigned kuasha420 Apr 12, 2022
@asvinb asvinb removed their assignment Apr 13, 2022
@eugene-manuilov eugene-manuilov removed their assignment Apr 15, 2022
@wpdarren wpdarren self-assigned this Apr 19, 2022
@wpdarren
Copy link
Collaborator

@kuasha420 I have assigned this back to Execution since you're investigating.

@kuasha420
Copy link
Contributor

There is a big slack discussion on the problem that @wpdarren found. I'm here documenting what went wrong and where we are now regarding this. Also what needs to be done to potentially resolve this issue.

What happened?

  • We used to have separate ExistingGTMPropertyNotice and ExistingTagNotice before ETS.
  • We merged them together in Simplify Analytics UX for existing tags and GTM properties #4703
    • At that point, if there was both GTM and UA tag, the message for UA would take precedence and shown.
  • In Implement Analytics snippet toggle(s) in setup flow if there is an existing tag #4913 and Show Analytics useSnippet toggle when creating a new UA/GA4 property #5045 we added Snippet toggle to the Setup flow of the Analytics(UA and GA4). We also copied the messaging related to Existing Tag below the toggle.
    • Note that, this existing tag related messages are only shown under Snippet Toggles in Setup flow. Settings is not changed.
    • At this point, the setup flow is showing two similar existing tag notices in Setup flow - one above the form and one under the toggles.
  • With this issue, we tried to take care of the duplicate Existing Tag messages by removing them from the top of the Setup screen by removing them from the ExistingTagNotice component.
    • This solves the duplicate message problem in Setup flow.
    • However, as we haven't implemented Existing Tag messages in the Snippet Toggle for Settings page, the Settings View and Settings Edit screens no longer has any notices related to Existing Tag.

Solution

  • Re-purpose current ExistingTagNotice as the ExistingGTMPropertyNotice which will be used directly on top of Setup Form.
  • Restore old combined (GTM and ET) ExistingTagNotice as the notice that will be shown on Settings(View/Edit) screens. We can refactor it to use ExistingGTMPropertyNotice when there is no UA and there is GTM.

Caveats
Copies need to be checked and confirmed/corrected.

@felixarntz I'm assigning this back to you. Please review my finding and proposed solution and let me know how to move forward. Cheers.

@kuasha420 kuasha420 assigned felixarntz and unassigned kuasha420 Apr 22, 2022
@FlicHollis FlicHollis added the Rollover Issues which role over to the next sprint label Apr 25, 2022
@felixarntz
Copy link
Member Author

felixarntz commented Apr 25, 2022

@kuasha420 We need to differentiate between the message about existing tags vs GTM properties. The latter must remain because the toggle doesn't cover it. The first one is now covered by the toggle in setup, so we don't need to show it as a separate message which would be a duplicate. So both setup and settings need to inform about GTM properties at the top if relevant.

I think one point of confusion is that for some reason the current ExistingTagNotice component is called "ExistingTagNotice", but now no longer informs about existing tags, but about GTM properties. So it should probably be renamed to "ExistingGTMPropertyNotice". This wasn't part of the ACs of course, but reviewing this again now, I think it makes things confusing. When renaming this to ExistingGTMPropertyNotice, it becomes more clear that this still needs to surface in both the setup flow and settings, because it has nothing to do with the toggle.

Regarding existing tags: We definitely need to maintain one way or the other in settings, informing about existing tags. My take on this is the following:

  • Eventually, we should use a message on the toggle like in setup.
  • It may be quicker for now to bring back the message in settings, if so, let's do that for now.

In other words:

  • Option A: If it's trivial to use the same toggle with existing tag message in settings (with the difference that in settings it should always display, not only when there's an existing tag), let's do that.
    • If we do that, let's still duplicate the wrapper component instead of using the same, basically to have something like SetupUseSnippetSwitch and SettingsUseSnippetSwitch. This way we can easily alter the copy because it may very well need to be slightly different in settings from setup.
  • Option B: If it's easier to bring back the notice on existing tags to settings and leave the toggle as it is today in settings, let's do that.

The main thing at this point is that the message remains there in one way or the other. I'll let you assess which one is more straightforward to implement. See also related #4934 (comment)

@kuasha420
Copy link
Contributor

@felixarntz I have created a follow up PR restoring the existing tag notice for settings page on top of the page as it was the old and already defined UX and identical to the Tag Manager Settings UX. However, I agree that we should revisit this in the future for both Analytics and Tag Manager.

Couple of questions:

  1. When both existingGTMProperty and existingTag are present, which notice should be shown on Settings?
  2. As users can override the default behavior, we may need to update copies such as Since this tag refers to the same property selected here, Site Kit will not place its own tag and rely on the existing one to something like Since this tag refers to the same property selected here, by default Site Kit will not place its own tag and rely on the existing one. or something like that.
  3. Or, we can update the messages based on the toggle state of the Use snippet switches as we do for Setup, but then we're better off putting them in toggle. In that case, I'd still prefer if we have a new well defined issue with copies and designs for Settings page.

What do you think? Cheers.

@kuasha420 kuasha420 assigned felixarntz and unassigned kuasha420 Apr 26, 2022
@felixarntz
Copy link
Member Author

@kuasha420 Thanks, that approach you currently have in the PR looks good for now, showing both notices as needed.

I will open a separate issue for finalizing this:

@felixarntz
Copy link
Member Author

@kuasha420 Can you please update the QA Brief based on the changes? It should probably clarify that the setup UI should now only have a notice on an existing property in the GTM container, while settings UI should still have both. In setup, existing tags information should only surface via the toggle description.

@kuasha420
Copy link
Contributor

@felixarntz I have updated the QA brief. Cheers.

@wpdarren
Copy link
Collaborator

wpdarren commented Apr 28, 2022

QA Update: ⚠️

@kuasha420 a few observations:

  1. Am I assuming correctly that the but no longer inform about "existing" tags in the AC does not apply to the Tag Manager settings and Analytics right now? As you can see from my screenshot the settings does have a note saying An existing tag was found on your site (GTM-PZMRZQD). and it doesn't really mention this in the AC or QAB.

In the settings page __
image

When editing settings
image

The same applies with Analytics here where I have a UA snippet in the header.php but have a different account selected with UA and GA4 properties.

image

  1. The text on the edit settings screen is Please select your Tag Manager account and container below. You can change these later in your settings. which does not make sense when I'm on the settings page. I am assuming that needs removing?

@kuasha420
Copy link
Contributor

@wpdarren you are correct, it's a bit confusing at the moment.

  1. The AC is only for Setup pages now. Settings is going to be addressed in Finalize Analytics & Tag Manager informing about existing tags #5143
  2. This message will also be modified/removed on the aforementioned issue.

This can be moved forward, just add a note that this only applies to Setup and we're having another go at it for Settings separately.

@kuasha420 kuasha420 removed their assignment Apr 28, 2022
@FlicHollis FlicHollis added the End of Sprint Issues which are due by the End of Sprint label Apr 29, 2022
@wpdarren
Copy link
Collaborator

QA Update: ✅

As per note above from Arafat, right now the testing around this is for the setup of Tag Manager and UA / GA4 analytics. The settings will be picked up in 5143.

Verified:

  • GTM Notices are show in the top of the Setup when relevant.
  • Existing Tag Notice for UA and GA4 will be shown on their respective toggles in Setup page.
  • The Updated Copy from the AC will be shown on all relevant places.

image

  • For thee ExistingTagNotice Throughout the Analytics setup flow, the minor copy adjustment is made:
    Please select the account information below. You can change this view later in your settings.
    Please select the account information below. You can change this later in your settings.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
End of Sprint Issues which are due by the End of Sprint Module: Analytics Google Analytics module related issues P0 High priority Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

7 participants