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

Add "temporarily hidden" badge to hidden audiences in the Selection Panel #9096

Open techanvil opened 1 month ago

techanvil commented 1 month ago

Feature Description

When an Audience Tile has been temporarily hidden, we should show a badge to clarify its status in the Selection Panel.


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

Acceptance criteria

Implementation Brief

Test coverage

QA Brief

  1. Enable audience segmentation feature and enable the visitor groups. Add the non Site Kit created audience to audience tile like "All visitors".

  2. In the tester plugin, set Analytics > Force Analytics report data to Zero. Also, set Analytics Partial Data > Force Analytics audience partial data state to last-28-days. Save changes.

  3. Go to Site Kit dashboard and to audience tile section. You will see the tile in collecting data status with the link to hide the tile temporarily ( "Temporarily hide" ).

  4. Click on the "Temporarily hide" link for "All Visitors", that will hide the tile.

  5. Open the audience selection panel. There should be a "Temporarily Hidden" badge with a tooltip.

  6. Ensure it matches the Figma Design and the text is according to AC.

Changelog entry

eugene-manuilov commented 1 month ago

Create a new component assets/js/modules/analytics-4/components/audience-segmentation/TemporaryHiddenBadge.js.

@ankitrox why do we need it? Your IB says that we need to create it but nothing where we should use it. If we don't need to use it, then we shouldn't add it.

In assets/js/components/SelectionPanel/SelectionPanelItem.js ... Render a BadgeWithTooltip component in the widget header if ... The audience is hidden.

The SelectionPanelItem component should know nothing about audiences.

PS: also not need to use the checkbox for each sub item in your IB, use it for top level groups of action items, for sub items that are part of those groups just use ordered/unordered list, for example:

ankitrox commented 1 month ago

Thank you for reviewing the IB @eugene-manuilov and highlighting the issues. I've updated the IB according to the mentioned points.

Also, I agree with your point that SelectionPanelItem should not be concerned about anything related to audience as it is a generic component, but as I took the reference of #8170 for this one, I found that this is also doing the same, which we can update IMO.

Maybe, @techanvil can comment about #8170. Thanks

techanvil commented 1 month ago

Thanks @eugene-manuilov and @ankitrox. That's a great point that SelectionPanelItem shouldn't know about audiences, and a detail I missed when reviewing the IB for #8170.

I have therefore tweaked the IB for #8170 to avoid this. As part of the amendment I've specified adding a badge prop to SelectionPanelItem which can be referenced in the IB for this issue.

eugene-manuilov commented 1 month ago

@ankitrox could you please fix the structure of your IB because right now it feels like changes in AudienceItem are a subtask of PartialDataBadge?

Refer PartialDataBadge component. A common component can be extracted out of it as BadgeWithTooltip which should accept label and tooltipTitle as props.

We need to be concrete what exactly needs to be done, if this is something that not really necessary, then no need to include it in IB.

A common component can be extracted out of it as BadgeWithTooltip which should accept label and tooltipTitle as props.

Why not to change the existing one to accept label and class name?

ankitrox commented 1 month ago

Thank you @eugene-manuilov . I have updated the IB and formatting.

In the IB I have mentioned to replace PartialDataBadge component with the more generic BadgeWithTooltip component and remove PartialDataBadge component.

eugene-manuilov commented 1 month ago

Ok, thanks, IB ✔️