-
Notifications
You must be signed in to change notification settings - Fork 295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add "temporarily hidden" badge to hidden audiences in the Selection Panel #9096
Comments
@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.
The 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:
|
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 Maybe, @techanvil can comment about #8170. |
Thanks @eugene-manuilov and @ankitrox. That's a great point that I have therefore tweaked the IB for #8170 to avoid this. As part of the amendment I've specified adding a |
@ankitrox could you please fix the structure of your IB because right now it feels like changes in
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.
Why not to change the existing one to accept label and class name? |
Thank you @eugene-manuilov . I have updated the IB and formatting. In the IB I have mentioned to replace |
Ok, thanks, IB ✔️ |
Hi @techanvil. Question about the tooltip copy in the ACs:
I believe the text is grammatically incorrect. I think the correct version would be:
Would you agree? |
Hi @nfmohit, thanks for spotting that! I didn't notice it when I copied the text from Figma. I've fixed the text in the AC/IB, and also asked Sigal to fix it over in Figma. |
Thank you @nfmohit and @techanvil I've updated the tooltip text and pushed the changes. |
Hi @techanvil. The text still has a "the this", which I think is incorrect. Could you recheck? |
Apologies @nfmohit, I missed that part! I've amended accordingly. Thanks for the 🦅 👀! |
QA Update
|
QA Update ✅New issue raised for the UX on clicking the badge here: #9421 Since the main use-case is working as expected for this ticket, we are moving this to approval ✅
|
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
BadgeWithTooltip
inassets/js/components
.label
andtooltipTitle
as props. Both should be of typestring
.className
props to add additional class names to the component wrapper. Usegooglesitekit-badge-with-tooltip
class as a generic one.label
and fortooltipTitle
useInfoTooltip
component with title prop set totooltipTitle
. ReferPartialDataBadge
component which does the same.PartialDataBadge
usage which should useBadgeWithTooltip
component.label
prop asPartial data
.tooltipTitle
should already be there, so use same one.className
asgooglesitekit-audience-segmentation-partial-data-badge
to retain the existing styles.PartialDataBadge
component from codebase as it is no longer needed.assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSelectionPanel/AudienceItem.js
isItemDismissed
selector, passaudience-tile-${slug}
to the selector. Collect result intemporarilyHidden
constant.BadgeWithTooltip
component as abadge
prop toSelectionPanelItem
component (refer Add the “New” badges to the Selection Panel #8170) whentemporarilyHidden
istrue
.label
as "Temporarily hidden" toBadgeWithTooltip
.tooltipTitle
asSite Kit is collecting data for this group. Once data is available the group will be added to your dashboard.
className
asgooglesitekit-audience-segmentation-temporary-hidden-badge
Test coverage
Update tests which may get failed due to update in
PartialDataBadge
component.Add tests for temporarily hidden badge check by setting the audience tile to be dismissed and zero data.
QA Brief
Enable audience segmentation feature and enable the visitor groups. Add the non Site Kit created audience to audience tile like "All visitors".
In the tester plugin, set
Analytics > Force Analytics report data
toZero
. Also, setAnalytics Partial Data > Force Analytics audience partial data state
tolast-28-days
. Save changes.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" ).
Click on the "Temporarily hide" link for "All Visitors", that will hide the tile.
Open the audience selection panel. There should be a "Temporarily Hidden" badge with a tooltip.
Ensure it matches the Figma Design and the text is according to AC.
Changelog entry
The text was updated successfully, but these errors were encountered: