google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.23k stars 279 forks source link

Improve availability of additional Analytics settings w/o UA connected #6875

Closed mohitwp closed 1 year ago

mohitwp commented 1 year ago

Bug Description

While testing Analytics settings view I've noticed that if we disabled the 'Place the Universal Analytics Code' toggle button then IP address and Exclude analytics code toggle options also get hide. But it is not the case with 'Place the Google Analytics 4 code' toggle option. There is no relation of this option with IP address and Exclude analytics code toggle options. Is this expected ?

Steps to reproduce

  1. Go to Analytics settings page.
  2. Click on edit.
  3. Enable/Disable 'Place the Universal Analytics Code' toggle button
  4. Compare it behavior with 'Place the Google Analytics 4 code' toggle option.

Screenshots

https://user-images.githubusercontent.com/94359491/231741022-36b4d4a3-8c76-4b6e-85a6-15c130fa9314.mp4

Additional Context


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

Acceptance criteria

Implementation Brief

Test Coverage

QA Brief

Changelog entry

techanvil commented 1 year ago

Note that the "Anonymize IP addresses" option is only relevant for UA - see https://support.google.com/analytics/answer/2763052?hl=en

Also, at present the "Exclude Analytics" option only works when UA is connected, however this is a known issue and is tracked in a separate Asana task.

nfmohit commented 1 year ago

Hi @marrrmarrr!

A question here regarding the "Anonymize IP addresses" option in the Analytics module settings. As @techanvil mentioned above, it is only relevant for UA, since in GA4 IP anonymization isn't required as IP addresses aren't logged or stored.

With this in mind, I worked on a PoC PR for this issue, where the "Anonymize IP addresses" option is disabled with a relevant note when the site only uses GA4 and not UA. Here's a screencast showing the output for the PoC PR:

1

However, after a chat with @aaemnnosttv about this, we think we should place such a notice (that with GA4, IP masking is not necessary) at all times regardless of whether UA is being used or not as a manner of encouraging the user to connect/use GA4. For example:

2

  1. What do you think we should do here?
  2. If we disable the switch when the site doesn't place a UA snippet, should the toggle be in the turned-on state or the turned-off state?
  3. Do you think it is necessary to change any other copy in this area?

Thanks so much!

aaemnnosttv commented 1 year ago

@nfmohit since the notice above shows the message about GA4 in all cases, I think the switch itself shouldn't be shown when UA isn't enabled. This would eliminate the duplication in that component as well in the GA4 case.

Perhaps we should adjust the label on the switch to include a "(Universal Analytics only)" suffix so that the control itself is clearly only relevant to that one, and then again, not show it if UA isn't enabled (in which case users would only see the notice about GA4).

In order to avoid a layout shift here, we could hide the toggle visually rather than removing it but even if there was a layout shift, it would actually be ok since this is in response to the user's action, similar to expanding an accordion.

nfmohit commented 1 year ago

Thank you for the direction, @aaemnnosttv! I have added the ACs accordingly.

aaemnnosttv commented 1 year ago

Thanks @nfmohit – let's revise the AC to be a bit more concise – they shouldn't be concerned with how things are now, but define what the desired outcome is. We can add a note below if it's particularly useful but the AC themselves should be focused on the requirements.

nfmohit commented 1 year ago

Thanks @nfmohit – let's revise the AC to be a bit more concise – they shouldn't be concerned with how things are now, but define what the desired outcome is. We can add a note below if it's particularly useful but the AC themselves should be focused on the requirements.

Understood and updated the ACs to be more concise. Please let me know if it looks good now, thank you @aaemnnosttv!

aaemnnosttv commented 1 year ago
  • The visibility of the IP Addresses, Exclude Analytics, and Google Ads sections (and settings within) in the Analytics settings should be changed so they show up if either the UA snippet switch or the GA4 snippet switch is turned on, if the ga4Reporting feature flag is enabled.

@nfmohit For Exclude Analytics specifically, this is independent of the snippet we place on the page. I'm not sure if it's implemented like this currently or not but this could be used to exclude analytics even for an existing tag since it's based on the selected property/measurement-ID

  • In Analytics SetupForm and SettingsEdit with the ga4Reporting feature flag on, it should be ensured that the UA snippet switch doesn't appear without a UA property selected.

We only show the snippet toggle in setup when there is an existing tag, otherwise it's implied/default that the snippet is enabled, so this should behave the same way.

  • Inside the IP Addresses section:

    • Before the switch component, display a <SettingsNotice /> component with TYPE_INFO and notice copy In Google Analytics 4, IP masking is not necessary since IP addresses are not logged or stored..
    • Display the switch component only if the UA snippet switch is turned on.
    • Add a suffix to the switch label so that it says Anonymize IP addresses (Universal Analytics only).

LGTM 👍

nfmohit commented 1 year ago

I have updated the ACs accordingly, thank you for the kind review, @aaemnnosttv!

eclarke1 commented 1 year ago

@aaemnnosttv whilst our focus this sprint is on P0s, this is one of the high priority P1s that would be great to get in this sprint if possible. With this in mind, could I please ask you to review this AC and see if we can get it progressed to IB?

aaemnnosttv commented 1 year ago

LGTM, thanks @nfmohit 👍

techanvil commented 1 year ago

Hey @aaemnnosttv, while reviewing the IB I've noticed a couple of minor points that relate to the AC.

Noting how the suggested changes would look across the Settings Edit/View pages, it strikes me a couple of small adjustments to the Settings View page would be in order:

What do you think? cc @nfmohit

To illustrate, here are some screen captures, including the rest of the settings for context.

Settings Edit:

image


Settings View, with IP anonymization enabled (UA enabled): image


Settings View, with UA disabled: image

aaemnnosttv commented 1 year ago

Thanks @techanvil – my replies below

  • When IP anonymization is enabled, I think it would be useful/consistent to also include the parentheses with "Universal Analytics only", i.e.:

    • IP addresses are being anonymized (Universal Analytics only)

Looking at the screenshot, I think for the settings view it would be perhaps more consistent to prefix the heading instead, similar to the few above it which are also UA-specific; i.e. Universal Analytics IP Address Anonymization. With this prefix, it would make sense to omit showing the section in the settings view if UA wasn't connected.

This of course challenges the layout of the edit view, where the IP Addresses section could then be merged into the UA settings above it (no longer being a separate section) although this would leave the GA4 notice in an odd place. We could of course move this notice into the GA4 fields above which would be more consistent with this change. What if we only added that notice if UA was connected? Otherwise it wouldn't really be necessary to highlight.

We should probably get @marrrmarrr 's input here as well but WDYT?

  • Secondly, this is very minor but the other sentences on the page don't end in a full stop, so the full stop on the end of this one looks a bit out of place and should probably be removed:

    • In Google Analytics 4, IP masking is not necessary since IP addresses are not logged or stored.

Correct, no full-stop 👍 I've updated these parts of the current AC.

techanvil commented 1 year ago

Looking at the screenshot, I think for the settings view it would be perhaps more consistent to prefix the heading instead, similar to the few above it which are also UA-specific; i.e. Universal Analytics IP Address Anonymization. With this prefix, it would make sense to omit showing the section in the settings view if UA wasn't connected.

This of course challenges the layout of the edit view, where the IP Addresses section could then be merged into the UA settings above it (no longer being a separate section) although this would leave the GA4 notice in an odd place. We could of course move this notice into the GA4 fields above which would be more consistent with this change. What if we only added that notice if UA was connected? Otherwise it wouldn't really be necessary to highlight.

We should probably get @marrrmarrr 's input here as well but WDYT?

Thanks @aaemnnosttv. On the settings view page, prefixing the header and then omitting the section when UA isn't connected SGTM. I also like the idea of moving the IP Anonymization switch into the UA section on the edit page.

However, on the edit page, when it comes to moving the GA4 IP Anonymization notice into the GA4 section - I did initially think this was a good idea and was in the process of mocking it up to better illustrate the proposal for Mariya, when I realised that by making the notice conditional on UA being connected, it results in a layout shift where the control the user presses actually moves as a result of that press, which is not nice UX. Here's a video:

https://github.com/google/site-kit-wp/assets/18395600/ab819c55-12df-46c8-a6b4-e30ed8629be3

It seems there are three obvious choices here:

Realistically I don't think we should always show the GA4 notice, so I find myself somewhat torn between option 1 and 3. On reflection, maybe still colocating the notice is the way to go. Or there could be a better solution I haven't considered. Interested to hear your thoughts.

Correct, no full-stop +1 I've updated these parts of the current AC.

Nice one :+1:

mxbclang commented 1 year ago

@aaemnnosttv :wave: We've missed this one for the current sprint, but want to make sure we take care of it in the next sprint. Can you please move this forward today? Thanks!

aaemnnosttv commented 1 year ago

Realistically I don't think we should always show the GA4 notice, so I find myself somewhat torn between option 1 and 3. On reflection, maybe still colocating the notice is the way to go. Or there could be a better solution I haven't considered. Interested to hear your thoughts.

The other option I can think of is to always render the GA4 notice, but toggle its CSS visibility based on the state of UA. That way it would still only be shown when relevant without introducing the overhead layout shift which we should definitely avoid. This would add a bit of a visual gap when hidden but that could be acceptable.

Otherwise I would keep the GA4 notice colocated with the UA switch.

techanvil commented 1 year ago

Thanks @aaemnnosttv, the visibility: hidden approach seems a good idea but again, I was mocking it up and noticed that the way the current AC are defined means there will be a layout shift anyway due to the Dashboard View section toggling in and out of the page.

https://github.com/google/site-kit-wp/assets/18395600/cac78c88-4c34-4015-a1ff-90c0c42de9fd

I tried applying visibility: hidden to the Dashboard View section, but the result does not look very nice with all of the empty space when UA is disabled:

https://github.com/google/site-kit-wp/assets/18395600/12c16f0a-d119-46c3-80f6-dc4e9bc7e7b7

This makes me wonder if we should consider yet another approach where we show the Dashboard View in a disabled state with some alternative text when UA is disabled, rather than hiding it completely.

Then again, maybe the shift from just toggling the Dashboard View in/out (i.e. the first of these videos) is not so jarring and we could live with it....

However with all of these various options I'm starting to think we need to involve @marrrmarrr in the conversation here, or could get Sigal involved for a UX perspective.

For the sake of completeness, here's how it could look with the notice colocated with the UA switch. On balance I think this would be the best approach of the three mockups, as it keeps the GA4 view logical and only adds the extra visual complexity when it's relevant.

https://github.com/google/site-kit-wp/assets/18395600/100ec744-ab18-4106-bc30-4004ba89eba4

What do you think?

mxbclang commented 1 year ago

@aaemnnosttv Can you please take a look at this one? Would like to get this into next sprint as it's one of our last P1 Highs for GA4 Reporting. Thanks!

aaemnnosttv commented 1 year ago

Thanks @techanvil – I think your most recent POC is probably the cleanest compromise.

The only thing we might also consider (potentially in a separate issue) is to always render the dashboard view toggle, but in a disabled state as you said so long as UA wasn't enabled (but still toggled on/off as appropriate). This would prevent the lower UA toggle from creating an overhead shift which is what really sours the UX.

Let's double check with @marrrmarrr and @sigal-teller here given the nature of these changes but I think this is probably the best way to go until UA is removed.

marrrmarrr commented 1 year ago

From a user perspective, I think "IP anonymisation" is more of a single topic, so it makes sense to have all notices/info related to it grouped together visually as well. Hence IMO the suggestion to co-locate the GA4 notice with the IP Anonymization switch would be a good way to go here.

sigal-teller commented 1 year ago

I agree that grouping related setting is better and prevent weird shifting. In any case this will be relevant for a short period of time as UA will be gone soon.

techanvil commented 1 year ago

Thanks @marrrmarrr and @sigal-teller. Sounds like colocating the GA4 notice is the way to go here.

There is an additional issue under consideration too. Even when colocating the notice, there's still a vertical layout shift when toggling the Enable Universal Analytics switch; this actually moves the control that the user is pressing (i.e. the switch) away from the cursor, which is not a great UX. This shift is due to the Dashboard View setting appearing/disappearing from the page. We're discussing whether it would be better to show the Dashboard View toggle in a disabled state (possibly with some alternative text while it's disabled) instead of removing it from the page, in order to prevent this vertical layout shift.

Can I ask for your thoughts on this aspect?

https://github.com/google/site-kit-wp/assets/18395600/100ec744-ab18-4106-bc30-4004ba89eba4

sigal-teller commented 1 year ago

@techanvil TBH this screen is not great, UX wise. There isn’t a clear hierarchy and it’s not clear that it’s divided into 2 distinguished section (UA + GA4). I assume that it was built once and then settings were added. We can put some effort in improving this but considering current priorities and the fact that UA will be gone soon we’ll probably won’t complete it by the time this screen will change again. For now I would choose the simplest solution here which is to keep all settings as they are and turn it to its disabled state. After UA is gone it might be a good idea to make sure that this screen is clear and well organized.

aaemnnosttv commented 1 year ago

For now I would choose the simplest solution here which is to keep all settings as they are and turn it to its disabled state. After UA is gone it might be a good idea to make sure that this screen is clear and well organized.

Thanks @sigal-teller – do you mean that we would keep the dashboard view setting present in all cases, but disabled whenever UA is not enabled rather than showing it conditionally?

sigal-teller commented 1 year ago

@aaemnnosttv That was @techanvil suggestion if I got it correctly, right? This shifting that happens above the toggle is really weird. As I said I would change this screen layout but it's not worth the effort for this short period of time.

techanvil commented 1 year ago

Thanks folks. I have updated the AC to reflect the changes we've discussed here, and also merged the changes from my local mockup into the PoC referenced in the IB so this is also up-to-date.

mohitwp commented 1 year ago

QA update ❌

@techanvil

The visibility condition of the Google Ads section (and settings within) should be changed so it shows up if either the UA snippet switch or the GA4 snippet switch is turned on

ISSUE Above scenario condition is implemented successfully when a/c account have both UA and GA4 properties. But, if analytics a/c have only GA4 property and if GA4 switch is turned off, then also Google Ads section is showing.

![image](https://github.com/google/site-kit-wp/assets/94359491/0f1a0c10-ed6a-410a-92aa-8ae646be5635) https://github.com/google/site-kit-wp/assets/94359491/65257bf6-7ec7-4fda-bece-799335fe7fca
techanvil commented 1 year ago

ISSUE Above scenario condition is implemented successfully when a/c account have both UA and GA4 properties. But, if analytics a/c have only GA4 property and if GA4 switch is turned off, then also Google Ads section is showing.

Thanks @mohitwp, good spot. I've created a PR to address this.

aaemnnosttv commented 1 year ago

Back to you for another pass @mohitwp 👍

mohitwp commented 1 year ago

QA Update ❌

@techanvil

Issue > I verified the issue reported above . Now if analytics a/c have only GA4 property and if GA4 switch is turned off, then Google Ads section is not showing under SettingsEdit but it still showing under SettingsView .

![image](https://github.com/google/site-kit-wp/assets/94359491/20176fd8-b45e-483b-ae75-34f0ef0876dd) ![image](https://github.com/google/site-kit-wp/assets/94359491/42e0da6a-f352-4fcc-a06e-d69890307671) https://github.com/google/site-kit-wp/assets/94359491/1450d5e8-05a4-4da5-86d1-b29d4ff6f9ca
aaemnnosttv commented 1 year ago

Thanks @mohitwp – back to you for another pass 👍

mohitwp commented 1 year ago

QA Update ✅

https://github.com/google/site-kit-wp/assets/94359491/f7cab426-18a3-4d44-a71c-3384576e3d75 https://github.com/google/site-kit-wp/assets/94359491/46c0c693-2575-4ce9-8a2a-bf7995c4c74c
aaemnnosttv commented 1 year ago

LGTM 👍