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 287 forks source link

Add and hook up the Change groups CTA, integrating the Selection Panel and its core happy path functionality #8158

Closed techanvil closed 3 months ago

techanvil commented 8 months ago

Feature Description

Add the Change groups CTA to the Audiences Widget Area and open the Selection Panel upon clicking it. Hook up the Selection Panel CTAs for saving and cancelling.

See audiences widget area > change groups CTA and selection panel in the design doc.


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

Acceptance criteria

Visibility and Functionality of Change Groups CTA

Selection Panel CTA Functionalities

Implementation Brief

Test Coverage

QA Brief

Changelog entry

eugene-manuilov commented 8 months ago

Selection Panel CTA Functionalities

@hussain-t, I believe the Selection Panel CTA is going to be implemented in another ticket, right? Let's add a reference to that ticket to make it easier to understand where to find it.

hussain-t commented 8 months ago

Thanks, @eugene-manuilov. This ticket is already blocked by #8157, which is to implement the selection panel. However, I have added a reference to the AC as well.

eugene-manuilov commented 8 months ago

Thanks, @hussain-t. AC ✔️

techanvil commented 8 months ago

Hi @zutigrm, thanks for drafting this IB.

A minor point - as per the new code organization section in the design doc, the SASS file for the component should be in assets/sass/components/analytics-4/audience-segmentation/.

More significantly, the determination of the available audiences is not as straightforward as simply retrieving the list from the API. The "Purchasers" default audiences required some special handling. This is discussed in the available audiences section in the design doc.

However, the implementation for this is not expected to be defined in this issue - rather it will be handled in https://github.com/google/site-kit-wp/issues/8157 which is a dependency of this issue.

I would imagine we'll need to get #8157 and notably its dependency #8141 defined before we can complete the IB for this one.

zutigrm commented 8 months ago

@techanvil Thanks, noted. I will hold off until two mentioned issues are defined before updating this one.

zutigrm commented 6 months ago

Unassigning myself as this will go to the squad 2. The dependencies mentioned in the last comment are still not defined

hussain-t commented 5 months ago

Hi @techanvil. The IB has been updated and is back with you for review.

techanvil commented 5 months ago

Thanks @hussain-t, this IB is generally looking good.

One thing though, as per my previous comment, we need to ensure that the list of available audiences takes into account the requirement for Purchasers to be "active" i.e. that it's had some users since the property was created.

The logic for determining this is still being defined in #8157 and we should be reusing the same logic here. I think we still need to wait until it's fully defined in #8157 before we can confidently complete the IB for this issue.

hussain-t commented 4 months ago

Thanks, @techanvil. The getAvailableAudiences selector is being modified for the abovementioned logic in #8157. I have added a note to the IB.

techanvil commented 4 months ago

Thanks @hussain-t! The IB LGTM. :white_check_mark:

nfmohit commented 4 months ago

@techanvil / @sigal-teller Quick question: Should we update the "Change Metrics" CTA (for KM) as part of this issue? I feel this may create an inconsistency if we don't. I've noticed two differences between the current state and the Figma designs linked here:

  1. The "Metrics" is capitalised in the current state, whereas it is lowercase in the Figma designs.
  2. The font-family is Google Sans Display in the current state, whereas it is Google Sans Text in the Figma designs.
sigal-teller commented 4 months ago

@nfmohit The "Metrics" shouldn't be capitalized. In CTA buttons we also capitalized only the first word. As for the font can you share a link to Figma where the font has changed so I can take a look? It should be "label medium" from the design system.

nfmohit commented 4 months ago

@nfmohit The "Metrics" shouldn't be capitalized. In CTA buttons we also capitalized only the first word. As for the font can you share a link to Figma where the font has changed so I can take a look? It should be "label medium" from the design system.

Thanks @sigal-teller! This is the Figma design I was referring to, specifically the "Change metrics" link. I was actually referring to the font face.

sigal-teller commented 4 months ago

Here are 2 screenshots of how it looks for me in Figma. Both CTAs have the same font style and size which is what defined in the design system as "Label/medium" which is defined as Google Sans Text. medium, 14

Screenshot 2024-06-10 at 17 49 48 Screenshot 2024-06-10 at 17 49 39
nfmohit commented 4 months ago

Got it, thank you @sigal-teller. I'll update the "Change metrics" link to be lowercase, and to use "Google Sans Text" according to the Figma designs and for consistency with the "Change groups" link.

kelvinballoo commented 4 months ago

QA Update ❌

I ran a test on this and I have some observations and queries. They are documented below

ITEM 1 - Clarification ⚠️ I am focusing on this point: The "Change groups" CTA should be visible within the Audiences Widget Area only when there is a non-zero number of available audiences. What are we implying by 'non-zero'? Does it mean as long as we have at least one audience? Or we are referring to the number of users in an audience between at least one?


ITEM 2 - Clarification ⚠️ Regarding the point : This CTA should not appear if an error occurs while retrieving available audiences. Is there a way to simulate an error on my end for testing this point?


ITEM 3 - Clarification ⚠️ For this point: Save Selection CTA: Upon clicking, this should dispatch the saveAudienceSettings() action to persist the local state of the audience settings via the POST:audience-settings endpoint.

I did see an audience-settings endpoint request being triggered. It's attached below with status 200. Is that the request we are to validate?

Screenshot 2024-06-11 at 21 39 11

ITEM 4 - Clarification ⚠️ My own created audience shows up as 'Already exists in your Analytics property'. Is that the correct string to be displayed? I did not see it in the design doc.

Screenshot 2024-06-11 at 19 04 15

ITEM 5 - Bug ❌ I linked up an analytics property and there are some users when I check it in Analytics. However, those numbers do not appear in the selection panel. All the numbers in the selection panel are 0.

Analytics shows values for the audiences: Screenshot 2024-06-11 at 19 09 05 Selection panel are 0. Please disregard the red selection on the screenshot: Screenshot 2024-06-11 at 19 04 15 Tiles are also showing 0: Screenshot 2024-06-11 at 19 15 58

ITEM 6 - Bug ❌ Here's my scenario:

Screenshot 2024-06-11 at 19 13 42

ITEM 7 - Visual Issue ❌ Distance between New badge and text should be 14px instead of 6px.

Implementation: 6px Screenshot 2024-06-11 at 22 05 59 Figma: 14px Screenshot 2024-06-11 at 22 05 10

ITEM 8 - Visual Issue ❌ Margin on top of text should be 32px and not 24px.

Implementation : 24px Screenshot 2024-06-11 at 18 22 35 Figma: 32px Screenshot 2024-06-11 at 22 08 17

ITEM 9 - Visual Issue ❌ Distance between pen icon and 'Change groups' text should be 5px from the pen. There should be no gap.

Implementation: notice that there is a gap before the 5px margin: Screenshot 2024-06-11 at 22 09 41 Figma: Screenshot 2024-06-11 at 22 11 11

ITEM 10 - Visual Issue ❌ The 'Change groups' should appear at the bottom of the text on mobile and tablet. Currently, it appears at the top.

Implementation: Screenshot 2024-06-11 at 19 17 43 Figma: Screenshot 2024-06-11 at 19 18 16
nfmohit commented 4 months ago

Hi @kelvinballoo, thank you for sharing your observations.

ITEM 1 - Clarification ⚠️ I am focusing on this point: The "Change groups" CTA should be visible within the Audiences Widget Area only when there is a non-zero number of available audiences. What are we implying by 'non-zero'? Does it mean as long as we have at least one audience? Or we are referring to the number of users in an audience between at least one?

It refers to the number of available audiences/groups, not the number of users.

ITEM 2 - Clarification ⚠️ Regarding the point : This CTA should not appear if an error occurs while retrieving available audiences. Is there a way to simulate an error on my end for testing this point?

It's slightly complicated to simulate an error, but you can simulate a state where there are no available audiences (which will basically result due to an error). To do so, run the following script in the browser console:

googlesitekit.data.dispatch( 'modules/analytics-4' ).setSettings( {
    availableAudiences: null,
} );

You should see that the CTA has disappeared.

ITEM 3 - Clarification ⚠️ For this point: Save Selection CTA: Upon clicking, this should dispatch the saveAudienceSettings() action to persist the local state of the audience settings via the POST:audience-settings endpoint.

I did see an audience-settings endpoint request being triggered. It's attached below with status 200. Is that the request we are to validate?

That is correct.

ITEM 4 - Clarification ⚠️ My own created audience shows up as 'Already exists in your Analytics property'. Is that the correct string to be displayed? I did not see it in the design doc.

That is the correct string according to the Figma designs.

ITEM 5 - Bug ❌ I linked up an analytics property and there are some users when I check it in Analytics. However, those numbers do not appear in the selection panel. All the numbers in the selection panel are 0.

Analytics information that is available in analytics.google.com does not reflect right away in Site Kit, as there is a 24-48 hour delay for it to be available in the reporting API. If the respective metrics are also zero in other parts of Site Kit (e.g. Audience tiles, according to your third screenshot in this section), it is expected for them to also be zero in the selection panel.

ITEM 6 - Bug ❌ Here's my scenario:

  • I set up an audience tile
  • I go to analytics and connect another property
  • I go back to my audience tile on the dashboard and the tile is still showing with the title being blank

This is indeed a bug, but out of the scope of this issue. Ideally, when an Analytics property is changed in Site Kit, the list of configured audiences should also be reset. Could you open a new issue regarding this? Thanks!

ITEM 7 - Visual Issue ❌ Distance between New badge and text should be 14px instead of 6px.

This is also out of the scope of this issue, as this issue is only concerned about the "Change groups" CTA button and the selection panel that it opens. However, I believe this uses the standard new badge placement procedure similar to any other widget which was likely set to 6 pixels before. If we had to change it, we'd have to change it in the widget area registration function so that it changes for any other widget area as well. You can open a separate issue for this as well.

ITEM 8 - Visual Issue ❌ Margin on top of text should be 32px and not 24px.

This is also out of the scope of this issue. You may open a separate issue for this.

ITEM 9 - Visual Issue ❌ Distance between pen icon and 'Change groups' text should be 5px from the pen. There should be no gap.

According to Figma (I'm using "Dev mode"), I can see that there's a 6 pixel gap. Screenshot:

image

The icon, when used during implementation, has a spacing of around 1 pixel around the subject to account for non-square graphics. Also, this is the same button that we use for the "Change metrics" CTA in the Key Metrics section if you scroll a little above in your Site Kit dashboard (with KM configured). If we had to change anything here, it'd lose consistency. In short, I believe this is fine.

ITEM 10 - Visual Issue ❌ The 'Change groups' should appear at the bottom of the text on mobile and tablet. Currently, it appears at the top.

This is something that I think needs a little confirmation from @sigal-teller and @techanvil. The CTA button here is attached to the widget area. We can make it show up in the footer of the widget area for mobile breakpoints (this will also apply globally and will impact the KM "Change metrics" CTA as well), but the Audience Segmentation widget area also has an additional InfoNoticeWidget underneath the audience tiles, so the "Change groups" CTA will appear below the InfoNoticeWidget. Is it okay to show it there, or do we want to do something else here?

Here's a mock of what it may look: image

@kelvinballoo, as this is a release issue and Tom & Sigal both appear to be OOO, would you mind opening a follow-up issue for this as well so that we can address this separately for both "Change groups" and "Change metrics" CTAs?

kelvinballoo commented 4 months ago

QA Update ⚠️

Thanks @nfmohit . Most of the items are good to go. Need your attention on item 5 and 10.

ITEM 1 - ✅ Noted. I have tried to test a scenario with 0 audience but it seems that this is not possible as there will always be a defaulted 'All Users' audience. That is fine.


ITEM 2 - ✅ Yes, indeed after executing the null script, the CTA would disappear.

googlesitekit.data.dispatch( 'modules/analytics-4' ).setSettings( {
    availableAudiences: null,
} );
CTA disappears after executing script. Screenshot 2024-06-12 at 15 21 07

ITEM 3 - ✅ Noted. No action here.


ITEM 4 - ✅ Got it. I looked in the wrong window with the many designs. Found out the correct design and it's as expected.

Figma wording is same as implemeted : "Already exists in your Analytics property" Screenshot 2024-06-12 at 15 24 39

ITEM 5 - ⚠️ I want to drill down a bit more on this as it's been already 24 hours since I spun up the site and in the last 28 days, there's been 3 users for 'All users' and 'New Visitors'. However, these are still not reflected in the selection panel, even if I set up my SK dashboard to last 28 days too.

Would you rather I wait another 24 hours @nfmohit to confirm if this is working or not? Or is this out of scope for this release?

3 users shown on GA: Screenshot 2024-06-12 at 15 30 46 0 shown on selection panel: Screenshot 2024-06-12 at 15 30 58

ITEM 6 - ✅ Issue raised here: https://github.com/google/site-kit-wp/issues/8860


ITEM 7 -
Issue raised here: https://github.com/google/site-kit-wp/issues/8861


ITEM 8 - ✅ Issue raised here: https://github.com/google/site-kit-wp/issues/8862


ITEM 9 -
Just to clarify what I was trying to say was that on the implementation, we have possibly a gap of 10px roughly from the pen to the CTA. On Figma, it's around 5px (if 6px from dev figma, that's fine) but there is this extra gap denoted in blue below. The orange section is 5px. The blue arrow is roughly around that size too, totalling 10px. (Image below)

That said, noted taken on what you mentioned about consistency when comparing with KM selection. The figma design is not accurate here.

Screenshot 2024-06-11 at 22 09 41 KM section: There is the gap as well: Screenshot 2024-06-12 at 16 00 12

ITEM 10 - ⚠️ Issue raised under : https://github.com/google/site-kit-wp/issues/8863 @nfmohit , I did not paste the comment you had over there because I did not understand everything. Also, not sure exactly whether to post in comments or somewhere in the ticket. So maybe you can follow up there with your comment.

kelvinballoo commented 4 months ago

QA Update ✅

All the items have been acted upon now.

Here's an update on the pending items in the earlier comment: ITEM 5 ✅ This is likely a setup issue. After using another property, the values were pulled on the selection panel and tiles.

Screenshot 2024-06-13 at 10 29 32 Screenshot 2024-06-13 at 10 44 18

ITEM 10 ✅ Comment has been updated in https://github.com/google/site-kit-wp/issues/8863 by Nahid


As for the points under the QAB, the following were already met accordingly:

Moving this ticket to Approval.

Change groups CTA is present: Screenshot 2024-06-11 at 18 04 02 Data is shown in the panel and on the tiles: Screenshot 2024-06-13 at 10 44 18 Screenshot 2024-06-13 at 10 29 32 Custom audiences appear as well: Screenshot 2024-06-11 at 19 03 04