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 the Selection Panel happy path view (Storybook) #8157

Closed techanvil closed 3 months ago

techanvil commented 8 months ago

Feature Description

Create the Selection Panel component, implementing the happy path view, and add it to Storybook.

This should include the CTAs but not the behaviour for saving the selection, this will be handled separately via https://github.com/google/site-kit-wp/issues/8158.

Additionally it should not include the loading state, the Audience Creation Notice or the Selection Panel Info Notice, these will be also handled separately via https://github.com/google/site-kit-wp/issues/8162, https://github.com/google/site-kit-wp/issues/8164 and https://github.com/google/site-kit-wp/issues/8159.

It should include the logic/behaviour described in the selection panel > available audiences section.

See selection panel in the design doc.


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

Acceptance criteria


Implementation Brief

Please refer POC PR: https://github.com/google/site-kit-wp/pull/8676. Also, this implementation will be done once #8652 is merged.

Test Coverage

QA Brief

Changelog entry

techanvil commented 8 months ago

Hey @hussain-t, just a heads up that I've amended the Feature Description here to make it clear that we do want to include the logic described in available audiences as part of this issue.

I've also tweaked that section in the design doc to point out that the implementation can leverage the "partial data" infrastructure, and as a result I've added https://github.com/google/site-kit-wp/issues/8141 to the dependencies for this issue. Cc @ivonac4

techanvil commented 6 months ago

Hi @hussain-t, just a heads up that I've made a small tweak to the AC to make it clear that we should be leveraging the audience cache here, as opposed to retrieving the audiences directly from the audiences API.

techanvil commented 6 months ago

Hi @hussain-t, I've also added a point to the AC to make it clear we don't need to address the specific details of audience ordering in this issue, this will be handled via #8519. This shouldn't actually have any impact on this issue, just want to give a heads up for clarity.

hussain-t commented 5 months ago

@ankitrox, could you set an estimate?

ankitrox commented 5 months ago

@hussain-t Added the estimate. Thank you.

techanvil commented 5 months ago

Hi @ankitrox, thanks for drafting this IB. Here are some points to consider:

techanvil commented 5 months ago

Note that since my previous comment we've decided to split this issue into two, with https://github.com/google/site-kit-wp/issues/8652 being created for the initial KM Selection Panel refactor.

ankitrox commented 5 months ago

Thank you @techanvil for breaking this task into refactoring and audience segmentation panel. This is helpful to better handle the overall requirement.

I have updated the IB considering the fact that we have a separate task for refactoring and key metrics panel. This IB take reference of your POC PR, but I think that should be fine because we are composing the components using extracted components.

Assigning this to you for review.

Thanks again!

techanvil commented 5 months ago

Hey @ankitrox, thanks for updating the IB. However - you've accidentally removed the AC and replaced the AC section with the new IB. Please can you restore the AC (you can see this in the edit history) and send it back to me to continue the review?

ankitrox commented 5 months ago

@techanvil Extremely sorry for the inconvenience! I have restored the AC from issue's history.

Assigning it over to you to continue the review.

Thanks

techanvil commented 5 months ago

Hi @ankitrox, no worries! These things happen, luckily we have the edit history to help us out here. Thanks for restoring the AC. As to the IB, it's generally looking good - here are a few points to address:


If there are no saved audiences, we should always display user created audiences which has non-zero user count, under "Current Selection". User count will be fetched by getReport selector in analytics module.


"All visitors" (which are "All users" in analytics and renamed in site kit), will be shown under "Additional groups" if not selected.


"Purchasers" will be shown in "current selection" if and only if it has user count for past 90 days and has not been archived.


It should leverage the SelectionPanelItem component and pass the necessary props to it. All these props should already be passed by SelectionPanelItems component down to this component.


Create AudienceCreationNotice component which will be displayed only if site kit created audiences i.e. New visitors and Returning visitors are not available in audiences returned by getAvailableAudiences selector.


Add the AudienceSegmentationPanel in storybook with the following scenarios.


  • The generic components extracted from the KM Selection Panel should be reused (see google/site-kit-wp#8652), with changes to the design for Audience Segmentation applied back to the KM panel (notably the footer layout in the mobile view as mentioned in the design doc).
ankitrox commented 5 months ago

Thank you so much @techanvil for the IB review.

I have made the suggested changes, but I have only one question regarding the following point.

We probably need to extend SelectionPanelItem to include a subtitle prop (as per the related point in this comment), and also some way of passing/rendering the user count (e.g. a "suffix" as we have discussed previously) - or, maybe the user count can be rendered directly via AudiencesItem. It's worth a mention, either way.

As per our extracted component SelectionPanelItem, we can pass react component for description and children, so essentially we can create a component which consist of description and subtitle and pass it to description prop.

Also, for user count we can pass a component as children by slightly modifying the SelectionPanelItems component to accept the children inside ItemComponent.

I have added this in IB, please let me know your thoughts.

Thank you.

techanvil commented 5 months ago

Hey @ankitrox, thanks for iterating on this IB. A few further points:


  • If there are no saved audiences...
  • All visitors" (which are "All users" in analytics and renamed in Site Kit), will be initially shown...
  • "Purchasers" will also be initially shown under "Additional groups"...

  • Note that we need to change SelectionPanelItems component a bit so that we can pass children to ItemComponent.
  • children passed to the ItemComponent can be a react component useful to display the user count for the audience.

https://github.com/google/site-kit-wp/blob/f6ea234136d2ca2e70aaf166e6694c2d0bfbed59/assets/js/components/SelectionPanel/SelectionPanelItems.js#L40-L45


It's worth noting that generic components extracted from the KM Selection Panel should be reused in audience selection panel. We may also need to apply some design changes; for example audience selection panel footer is slightly different than the KM selection panel footer (in mobile view). In such cases, we can target the particular elements by passing appropriate class names and styling them differently in CSS. We may also need slightly different components to be passed, for example; in SelectionItem we may need different components for description and children.


Please let me know if it would be useful to jump on a call to discuss/clarify any of the above, I realise it's quite a complicated one that might benefit from a sync.

techanvil commented 5 months ago

Thanks for updating the IB, @ankitrox. It's LGTM (please note I made a small tweak to the Test Coverage section which I felt could be a little less explicit).

IB :white_check_mark:

techanvil commented 4 months ago

Hi @nfmohit, nice work here. I've just merged the PR.

However - considering this issue was only intended to be scoped to Storybook, I am questioning if we need to include the integration testing in the QAB. I note that you've rendered AudienceSelectionPanel in DashboardMainApp which I can imagine was useful for additional testing during development, and I've left that in, but really it's a bit out of scope here. Considering the issue has gone over the estimate I'd suggest removing the E2E tests from the QAB and instead use those as a basis for the QAB for #8158 which I see you currently have in execution. WDYT?

nfmohit commented 4 months ago

Hi @nfmohit, nice work here. I've just merged the PR.

However - considering this issue was only intended to be scoped to Storybook, I am questioning if we need to include the integration testing in the QAB. I note that you've rendered AudienceSelectionPanel in DashboardMainApp which I can imagine was useful for additional testing during development, and I've left that in, but really it's a bit out of scope here. Considering the issue has gone over the estimate I'd suggest removing the E2E tests from the QAB and instead use those as a basis for the QAB for #8158 which I see you currently have in execution. WDYT?

That's a good call, thank you @techanvil. I've moved out all the plugin testing from the QAB and just left the parts for Storybook. Let me know what you think, thank you!

techanvil commented 4 months ago

Thanks @nfmohit, that looks great. Over to QA for testing :)

kelvinballoo commented 4 months ago

QA Update ⚠️

Tested and I have a few observations and queries, documented as follows:

ITEM 1: All visitors' should be 'Google Sans Text' instead of 'Google Sans Display'

Storybook: ![Uploading Screenshot 2024-06-06 at 16.11.57.png…]() Figma: Screenshot 2024-06-06 at 16 12 54

ITEM 2: People who visited the site for the first time font weight should be 400 instead of 500.

Storybook: Screenshot 2024-06-06 at 16 14 20 Figma: Screenshot 2024-06-06 at 16 15 31

ITEM 3: Font weight for 'Save selection' button should be 500 instead of 400. That said, I did see that the KM panel would be 400. So if this is to keep things consistent with KM, then we can ignore this.

Storybook: Screenshot 2024-06-06 at 16 17 41 Figma: Screenshot 2024-06-06 at 16 17 57

ITEM 4: There are a few points in the AC that I don't think can be tested in this Storybook ticket as the Storybook data are not real data linked to a real account. I've listed them below. Can you confirm that indeed these are items not to be tested in this ticket

nfmohit commented 4 months ago

Thank you for sharing your observations, @kelvinballoo !

ITEM 1: All visitors' should be 'Google Sans Text' instead of 'Google Sans Display'

Storybook: ![Uploading Screenshot 2024-06-06 at 16.11.57.png…]() Figma: Screenshot 2024-06-06 at 16 12 54

This is consistent with what we have in the Key Metrics selection panel. If we should change the font here, I believe we should change it in the KM panel as well for consistency. @sigal-teller Could you confirm if the font should be changed here to Google Sans Text for both KM and audience selection panels?

ITEM 2: People who visited the site for the first time font weight should be 400 instead of 500.

I'm opening a follow-up PR for this.

ITEM 3: Font weight for 'Save selection' button should be 500 instead of 400. That said, I did see that the KM panel would be 400. So if this is to keep things consistent with KM, then we can ignore this.

As you mentioned, this uses the re-usable component that both KM and audience selection panels use, so if we want to change it here, we should change it everywhere else for consistency.. @sigal-teller We need your confirmation here as well to see if this change should be applied for all selection panels. Thanks!

  • The Selection Panel should display the list of available audiences that is cached in the DB (i.e. stored in the availableAudiences setting). Their display names and descriptions should be displayed directly, with the exception of the "All users" default audience which should be shown as "All visitors".
  • The “Purchasers” default audience should only be listed if it’s had any users in it since the associated Analytics property was created.

As this is a Storybook-only issue, unfortunately, these cannot be tested here, but can be done in #8158. I'll add them in the QAB for #8158.

  • The user count for an audience should be displayed on the right hand side of it.

You should be able to see the user count from the mock report data in Storybook.

image

You'll be able to see actual data in #8158.

The Selection Panel should display the header text including the link to the Admin Settings tab on the Settings screen.

  • If the Audience Segmentation Settings section is available at implementation time this link should ensure the section is scrolled into view when navigating to the Settings screen. Otherwise, a small followup issue should be created to update the link when the section is available."

The link is correct, however, unfortunately, you'll not be able to see it in Storybook. It should be verifiable in #8158.

The link doesn't scroll down to the respective section though. @techanvil mentioned he is going to open an issue for it.

Thanks!

nfmohit commented 4 months ago

I have opened a follow-up PR #8829 to address some of the concerns above. For this to move forward, we have two questions for @sigal-teller that we need for confirmation:

1.

ITEM 1: All visitors' should be 'Google Sans Text' instead of 'Google Sans Display'

Storybook: image Figma: image

This is consistent with what we have in the Key Metrics selection panel. If we should change the font here, I believe we should change it in the KM panel as well for consistency. @sigal-teller Could you confirm if the font should be changed here to Google Sans Text for both KM and audience selection panels?

2.

ITEM 3: Font weight for 'Save selection' button should be 500 instead of 400. That said, I did see that the KM panel would be 400. So if this is to keep things consistent with KM, then we can ignore this.

As you mentioned, this uses the re-usable component that both KM and audience selection panels use, so if we want to change it here, we should change it everywhere else for consistency.. @sigal-teller We need your confirmation here as well to see if this change should be applied for all selection panels. Thanks!

Thank you!

sigal-teller commented 4 months ago

@nfmohit The links you added appears broken for me so I can see exactly what you're referring to. In any case, I'm using the styles that we have in the design system, so I'm not sure why the font is changing for you. Maybe there are missing fonts or styles for you and this is why replaces them? Anyway, group names (if that was what you were referring to) are label medium. LMK if you were referring to something else.

nfmohit commented 4 months ago

Hi @sigal-teller. I have updated the links for you, thank you for pointing that out. In summary, we have two questions:

1st: Should we use "Google Sans Text" for the checkbox labels in the selection panel? The KM panel, when implemented, used the "Google Sans Display" font, but in the current Figma designs for Audience Segmentation, it uses "Google Sans Text".

Screenshots In the current KM selection panel: ![image](https://github.com/google/site-kit-wp/assets/20284937/ca420901-f529-483c-83ef-152c3728fe26) In Audience Segmentation Figma designs: ![image](https://github.com/google/site-kit-wp/assets/20284937/9aae9edc-2448-43e2-99fe-f1bcee603d3d)

2nd: Should the font-weight for the "Save selection" CTA be 400 or 500. In the KM selection panel, it is 400, but in the Audience Segmentation Figma designs, it is 500.

Screenshots In the current KM selection panel: ![image](https://github.com/google/site-kit-wp/assets/20284937/a6ea0924-10c4-4960-8b49-330c03581868) In the Audience Segmentation Figma designs: ![image](https://github.com/google/site-kit-wp/assets/20284937/333d7310-fc89-480e-855d-a7a49c403f87)
sigal-teller commented 4 months ago

Hi @nfmohit

  1. Checkboxes labels should be "Google sans text" (this what is used for Label/medium" in Figma).
  2. font weight for buttons should be 500 (buttons text are Label/medium as well). you can find them in the design system here. Also attaching a screenshot. Screenshot 2024-06-11 at 16 57 35
nfmohit commented 4 months ago

Thank you for confirming, @sigal-teller. I'll make these updates accordingly.

nfmohit commented 4 months ago

@kelvinballoo This is now in CR with PR #8829 that addresses your observations: ITEM 1 & ITEM 2.

For ITEM 3, I have opened a new issue #8856.

The other questions have been addressed in my previous comments.

Thanks!

kuasha420 commented 4 months ago

@kelvinballoo Back to you for another pass, the ITEM 1 and 2 should now be fixed. As noted by @nfmohit above, the 3rd item is a much bigger change, so it will be fixed in a follow up issue.

kelvinballoo commented 4 months ago

QA Update ✅

All the issues are resolved or have an action in another ticket. Moving this ticket to 'Approval'.

ITEM 1: ✅ All visitors' is now 'Google Sans Text'.

Screenshot 2024-06-12 at 17 20 20

ITEM 2:People who visited the site for the first time font weight is now 400.

Screenshot 2024-06-12 at 17 22 12

ITEM 3: ✅ Noted that this will be handled under https://github.com/google/site-kit-wp/issues/8856.


ITEM 4: ✅ Noted that this is out of scope for this test.