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

Implement the “no audiences” banner happy path view #8155

Closed techanvil closed 2 months ago

techanvil commented 8 months ago

Feature Description

Implement the banners shown in the scenarios where all of the selected audiences have been archived in Analytics. This includes both variants of the banner that will be shown to the primary user.

The error state and the secondary user variants of the banner will be implemented separately, see https://github.com/google/site-kit-wp/issues/8190 and https://github.com/google/site-kit-wp/issues/8577.

See no audiences banner in the design doc.


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

Acceptance criteria


Implementation Brief

Test Coverage

  1. Add the tests for NoAudienceBannerWidget, ConfigurableNoAudienceBannerContent and NonConfigurableNoAudienceBannerContent components.
  2. Add the tests for AudienceTiles component.
  3. Update tests for AudienceTilesWidget component as we need to test whether it is returning appropriate components from it based on audiences availability and dashboard view context.
  4. Update tests for AudienceTilesWidget component as we need to test whether it is returning WidgetNull when no configured audiences available anymore.
  5. Fix any other failing VRTs.

QA Brief

TIP: It may be necessary to sync available audiences after archiving/deleting them in Analytics. You can force this using the following snippet.

await googlesitekit.data.dispatch('modules/analytics-4').syncAvailableAudiences()

For Quickly testing the banner. You can set configured audience to a non-existent one.

await googlesitekit.data.dispatch('modules/analytics-4').setConfiguredAudiences(['garboge'])

And for seeing the alternate version, sett the available audiences to an empty array so no audience is available.

await googlesitekit.data.dispatch('modules/analytics-4').setAvailableAudiences([])

Changelog entry

techanvil commented 6 months ago

I've moved this back to Backlog as the final in-progress changes to the design doc, relating to audience caching, will probably affect the AC for this one

techanvil commented 5 months ago

The audience caching aspect of the design doc has been sufficiently finalised, and I've moved this back to AC.

kuasha420 commented 3 months ago

Hey @ankitrox, Thanks for drafting the IB. A few suggestions:

Instead of baking this inside AudienceTilesWidget, what about we create a separate NoAudienceBannerWidget widget, and we can encapsulate the logic for the banner with/without configurable audiences inside this new widget.

The implication is that we need to render WidgetNull from either widget when they're not relevant, ie. return WidgetNull when configured audiences are not available from AudienceTilesWidget and reverse for NoAudienceBannerWidget.

Also instead of having two banners, we can keep the logic in a single one, since the difference is only copy and link. Should be manageable in one place.

Finally, some styling and layout ideas can be borrowed from ConnecAnalyticsCTA which is being implemented in #8156 (PR) since they're very similar.

Cheers.

ankitrox commented 3 months ago

Thank you for reviewing the IB @kuasha420

Adding the NoAudienceBannerWidget seems like a good approach. I have updated the IB to include that component, using this will no longer need a new AudienceTiles component so I removed it.

However, I think we should keep the ConfiguarblesNoAudienceBanner and NonConfiguarblesNoAudienceBanner components separate just to keep them simpler. Earlier, a similar feedback has been received in IBR from @techanvil on #8143

Assigning this to you for re-review.

Thanks again!

kuasha420 commented 3 months ago

Hi @ankitrox, Thank you for updating the IB. Looking pretty good. Just a couple of points:

  1. A separate AudienceTiles component may still provide some value. Essentially, we can rename the current AudienceTilesWidget to AudienceTiles, and keep the reporting and other logic inside it, and render the markup without the Widget wrapper. And in the "new" AudienceTilesWidget we will essentially render the AudienceTiles wrapped in Widget if the audiences are available, and WidgetNull otherwise. This will probably save us from making bunch of API requests and getting hit with API errors when audiences are not available.
  2. I'm not super opposed to separate ConfiguarblesNoAudienceBanner and NonConfiguarblesNoAudienceBanner, but seeing that their design is identical, and the logic to determine the difference in copies relies on same thing, I don't see a lot of value in it. We can probably make the separate components for the inner parts, which will essentially be just UI components, and keep the styling and layout in NoAudienceBanner, but doing that vs a couple of conditions in JSX markup doesn't really make much of a difference.
  3. Finally, if we decide to keep the separate components, the suggested name should be something like ConfiguarbleNoAudienceBannerContent and NonConfiguarbleNoAudienceBannerContent, to make it clear that they only render the inner content, also not the singular term Configuarble.

Cheers.

ankitrox commented 3 months ago

Thank you @kuasha420

I have made the necessary changes as suggested, but kept the configurable and non-configurable components separate with the changes in name as suggested.

Assigning this to you for re-review.

Cheers!

kuasha420 commented 3 months ago

Thanks @ankitrox. IB LGTM. :white_check_mark:

techanvil commented 3 months ago

Thanks guys! Just a heads up, I tweaked the IB to correct the "Configuarble" -> "Configurable" typo.

nfmohit commented 2 months ago

Hi @kuasha420 . As discussed internally, this issue is back with you to review if its PR needs any changes post the reversal of pivot reports. The AudienceTilesWidget component resulted in significant changes and your PR changes its file structure. Thank you!

kelvinballoo commented 2 months ago

QA Update ⚠️

I tried to do the first test about archiving one audience.

I have attached a video for reference as well:

https://github.com/user-attachments/assets/e07c6335-7a5e-4c3d-bf3b-60620f328336
techanvil commented 2 months ago

Hi @kelvinballoo, thanks for flagging this. These points are known issues, and we will be addressing them via https://github.com/google/site-kit-wp/issues/8145, ensuring the list of selected audiences is updated following the sync.

kelvinballoo commented 2 months ago

QA Update ⚠️

I've tested this and I have 2 items to be reviewed: ITEM 1: The 'settings' CTA on the banner would rightly link to the settings page of SK. However, I would suggest we could deeplink it to the actual setting so that the user doesn't need to scroll down and figure out where it is. That would be a better UX.

ITEM 2: Currently, based on my testing, I could not archive all the audiences because one will ultimately remain e.g. all users. Hence, I could not verify this from a user point of view. I only verified this through the script in the QAB which would blank all audiences. Would that be any concern if we don't test it practically?

Notice how only the 'Returning Visitors' have the 3 dots and hence the option to be archived. Screenshot 2024-07-22 at 13 59 52 Screenshot 2024-07-22 at 14 00 15

Other than that, the other areas are tested good:

techanvil commented 2 months ago

Hi @kelvinballoo, thanks for reviewing this and raising these issues.

I've tested this and I have 2 items to be reviewed: ITEM 1: The 'settings' CTA on the banner would rightly link to the settings page of SK. However, I would suggest we could deeplink it to the actual setting so that the user doesn't need to scroll down and figure out where it is. That would be a better UX.

Good spot, and agreed, it will be better UX as you've described. We do already have an issue for this, https://github.com/google/site-kit-wp/issues/8875 - it's a P2 though as it's not essential for launch.

ITEM 2: Currently, based on my testing, I could not archive all the audiences because one will ultimately remain e.g. all users. Hence, I could not verify this from a user point of view. I only verified this through the script in the QAB which would blank all audiences. Would that be any concern if we don't test it practically?

Another good point to raise. I was debating whether to leave this aspect in. I'm pretty sure I was at one point able to remove all audiences including the "All users" default. However in recent testing, it's not possible to remove it. I figured that we can leave this edge case in for now as it's a small addition and will be a robust way to handle the scenario if it does occur again for one reason or another. In the meantime, testing it programmatically by using the snippet in the QAB is the way to go here.

kelvinballoo commented 2 months ago

QA Update ✅

Thanks @techanvil for checking. We are good to go on this issue.