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

ankitrox · 2024-09-09T06:57:11Z

Summary

Addresses issue:

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

Relevant technical choices

PR Author Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 5.2 and PHP 7.4.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have added a QA Brief on the issue linked above.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

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

Code Reviewer Checklist

Run the code.
Ensure the acceptance criteria are satisfied.
Reassess the implementation with the IB.
Ensure no unrelated changes are included.
Ensure CI checks pass.
Check Storybook where applicable.
Ensure there is a QA Brief.

Merge Reviewer Checklist

Ensure the PR has the correct target branch.
Double-check that the PR is okay to be merged.
Ensure the corresponding issue has a ZenHub release assigned.
Add a changelog message to the issue.

github-actions · 2024-09-09T07:04:08Z

Build files for 8c01dd5 have been deleted.

github-actions · 2024-09-09T07:04:44Z

Size Change: +50.3 kB (+2.78%)

Total Size: 1.86 MB

Filename	Size	Change
`./dist/assets/css/googlesitekit-admin-css-********************.min.css`	58.1 kB	+503 B (+0.87%)
`./dist/assets/js/35-********************.js`	892 B	-1 B (-0.11%)
`./dist/assets/js/googlesitekit-activation-********************.js`	23.9 kB	+30 B (+0.13%)
`./dist/assets/js/googlesitekit-ad-blocking-recovery-********************.js`	72.5 kB	+86 B (+0.12%)
`./dist/assets/js/googlesitekit-adminbar-********************.js`	34.6 kB	+30 B (+0.09%)
`./dist/assets/js/googlesitekit-api-********************.js`	9.96 kB	-4 B (-0.04%)
`./dist/assets/js/googlesitekit-components-gm2-********************.js`	5.97 kB	-1 B (-0.02%)
`./dist/assets/js/googlesitekit-components-gm3-********************.js`	10.1 kB	+2 B (+0.02%)
`./dist/assets/js/googlesitekit-data-********************.js`	2.37 kB	-1 B (-0.04%)
`./dist/assets/js/googlesitekit-datastore-forms-********************.js`	8.95 kB	-3 B (-0.03%)
`./dist/assets/js/googlesitekit-datastore-location-********************.js`	2.08 kB	+1 B (+0.05%)
`./dist/assets/js/googlesitekit-datastore-site-********************.js`	20.4 kB	+22 B (+0.11%)
`./dist/assets/js/googlesitekit-datastore-ui-********************.js`	9.92 kB	+5 B (+0.05%)
`./dist/assets/js/googlesitekit-datastore-user-********************.js`	26.2 kB	+13 B (+0.05%)
`./dist/assets/js/googlesitekit-entity-dashboard-********************.js`	83.4 kB	-18 B (-0.02%)
`./dist/assets/js/googlesitekit-main-dashboard-********************.js`	158 kB	+1.18 kB (+0.75%)
`./dist/assets/js/googlesitekit-modules-********************.js`	22.1 kB	+3 B (+0.01%)
`./dist/assets/js/googlesitekit-modules-ads-********************.js`	32.7 kB	+94 B (+0.29%)
`./dist/assets/js/googlesitekit-modules-adsense-********************.js`	125 kB	+182 B (+0.15%)
`./dist/assets/js/googlesitekit-modules-analytics-4-********************.js`	172 kB	+838 B (+0.49%)
`./dist/assets/js/googlesitekit-modules-pagespeed-insights-********************.js`	22.5 kB	+55 B (+0.24%)
`./dist/assets/js/googlesitekit-modules-reader-revenue-manager-********************.js`	21.9 kB	+31 B (+0.14%)
`./dist/assets/js/googlesitekit-modules-search-console-********************.js`	58.8 kB	+35 B (+0.06%)
`./dist/assets/js/googlesitekit-modules-tagmanager-********************.js`	32 kB	-10 B (-0.03%)
`./dist/assets/js/googlesitekit-notifications-********************.js`	18.6 kB	+7 B (+0.04%)
`./dist/assets/js/googlesitekit-polyfills-********************.js`	377 B	-1 B (-0.26%)
`./dist/assets/js/googlesitekit-settings-********************.js`	129 kB	+46.1 kB (+55.6%)	🆘
`./dist/assets/js/googlesitekit-splash-********************.js`	90.5 kB	+16 B (+0.02%)
`./dist/assets/js/googlesitekit-user-input-********************.js`	64.3 kB	+87 B (+0.14%)
`./dist/assets/js/googlesitekit-vendor-********************.js`	321 kB	-29 B (-0.01%)
`./dist/assets/js/googlesitekit-widgets-********************.js`	88.6 kB	+964 B (+1.1%)
`./dist/assets/js/googlesitekit-wp-dashboard-********************.js`	62 kB	+165 B (+0.27%)
`./dist/assets/js/runtime-********************.js`	1.3 kB	-2 B (-0.15%)

ℹ️ View Unchanged

Filename	Size
`./dist/assets/css/googlesitekit-adminbar-css-********************.min.css`	11.8 kB
`./dist/assets/css/googlesitekit-authorize-application-css-********************.min.css`	846 B
`./dist/assets/css/googlesitekit-wp-dashboard-css-********************.min.css`	8.2 kB
`./dist/assets/js/31-********************.js`	2.76 kB
`./dist/assets/js/32-********************.js`	2.25 kB
`./dist/assets/js/33-********************.js`	3.64 kB
`./dist/assets/js/34-********************.js`	935 B
`./dist/assets/js/36-********************.js`	3.12 kB
`./dist/assets/js/analytics-advanced-tracking-********************.js`	901 B
`./dist/assets/js/googlesitekit-consent-mode-********************.js`	25.6 kB
`./dist/assets/js/googlesitekit-events-provider-contact-form-7-********************.js`	646 B
`./dist/assets/js/googlesitekit-events-provider-easy-digital-downloads-********************.js`	624 B
`./dist/assets/js/googlesitekit-events-provider-mailchimp-********************.js`	630 B
`./dist/assets/js/googlesitekit-events-provider-ninja-forms-********************.js`	712 B
`./dist/assets/js/googlesitekit-events-provider-optin-monster-********************.js`	675 B
`./dist/assets/js/googlesitekit-events-provider-popup-maker-********************.js`	634 B
`./dist/assets/js/googlesitekit-events-provider-woocommerce-********************.js`	657 B
`./dist/assets/js/googlesitekit-events-provider-wpforms-********************.js`	633 B
`./dist/assets/js/googlesitekit-i18n-********************.js`	3.93 kB

_{compressed-size-action}

…adge.

benbowler

Hey @ankitrox, this looks good and works well. I've added a couple of small details around the stories to commit if you agree.

benbowler · 2024-09-17T11:23:53Z

assets/js/components/BadgeWithTooltip.stories.js

 	tooltipTitle:
 		'Still collecting full data for this timeframe, partial data is displayed for this group',
 };
 Default.scenario = {
-	label: 'Modules/Analytics4/Components/AudienceSegmentation/Dashboard/PartialDataBadge/Default',
+	label: 'Modules/Analytics4/Components/BadgeWithTooltip/Default',


Suggested change

label: 'Modules/Analytics4/Components/BadgeWithTooltip/Default',

label: 'Components/BadgeWithTooltip/Default',

benbowler · 2024-09-17T11:24:48Z

assets/js/components/BadgeWithTooltip.stories.js

 };

 export default {
-	title: 'Modules/Analytics4/Components/AudienceSegmentation/Dashboard/PartialDataBadge',
+	title: 'Modules/Analytics4/Components/BadgeWithTooltip',


Suggested change

title: 'Modules/Analytics4/Components/BadgeWithTooltip',

title: 'Components/BadgeWithTooltip',

Now that you've made this a nice reusable component, it doesn't need to be used just in Analytics so you can move it to the main components section of the stories.

benbowler · 2024-09-17T11:27:21Z

assets/js/components/BadgeWithTooltip.stories.js

 }

 export const Default = Template.bind( {} );
 Default.storyName = 'Default';

 Default.args = {
+	label: 'Title for badge with tooltip',
 	tooltipTitle:
 		'Still collecting full data for this timeframe, partial data is displayed for this group',


Suggested change

'Still collecting full data for this timeframe, partial data is displayed for this group',

'This is an example of tooltip content for a badge with tooltip',

This is a small detail.

ankitrox · 2024-09-18T07:04:23Z

@benbowler Thanks for review and adding the feedback.

As Evan pointed out in a recent review, we don't actually need to specify the label for scenarios, as it is by default determined from the default title and the story's storyName, so I have skipped that point.

Rest of the points addressed, tested and pushed the changes in PR.

benbowler

Thanks @ankitrox, LGTM ✅

nfmohit

Excellent work on this, thank you @ankitrox !

I've left a few comments in the PR.
According to the IB, we should add some test coverage for the appearance of the "Temporarily hidden" badge.
1. Can we add one test case that verifies that the "Temporarily hidden" badge appears when conditions are met?
2. Can we add one test case that verifies that the "New" badge does not appear when the "Temporarily hidden" badge is visible?

Please let me know if you have any questions, thank you!

...nalytics-4/components/audience-segmentation/dashboard/AudienceSelectionPanel/AudienceItem.js

assets/js/components/BadgeWithTooltip.js

assets/sass/components/analytics-4/audience-segmentation/_googlesitekit-badge-with-tooltip.scss

assets/sass/components/analytics-4/_index.scss

...oogle-site-kit_Modules_Analytics4_Components_BadgeWithTooltip_Default_0_document_0_small.png

...ogle-site-kit_Modules_Analytics4_Components_BadgeWithTooltip_Default_0_document_1_medium.png

...oogle-site-kit_Modules_Analytics4_Components_BadgeWithTooltip_Default_0_document_2_large.png

nfmohit

Sorry, forgot to include this one in the original review above.

assets/sass/components/analytics-4/audience-segmentation/_googlesitekit-badge-with-tooltip.scss

…adge.

nfmohit

Hi @ankitrox . Thank you for updating the tooltip text. Maybe it didn't catch your attention but that wasn't the only feedback in my code review. Could you please take a look at my comments above and address them?

Moreover, I feel there is an additional update required to the tooltip text. I've asked Tom here.

I understand that we're under a time constraint here but I believe the changes requested are quite minimal. I'd have gone and applied them myself but since we're waiting for an AC change confirmation, I'm sending this back to Execution. CC: @techanvil @ivonac4

...nalytics-4/components/audience-segmentation/dashboard/AudienceSelectionPanel/AudienceItem.js

techanvil · 2024-09-20T23:11:01Z

Moreover, I feel there is an additional update required to the tooltip text. I've asked Tom here.

I understand that we're under a time constraint here but I believe the changes requested are quite minimal. I'd have gone and applied them myself but since we're waiting for an AC change confirmation, I'm sending this back to Execution. CC: @techanvil @ivonac4

Thanks for spotting that the text still needed an update! I've applied that change myself but see there are other comments for @ankitrox still to address, so I've left this in execution.

ankitrox · 2024-09-24T16:39:31Z

Thank you @techanvil for correcting the text and committing the change.

@nfmohit I've addressed the other comments. Sorry for missing them at the first place.

Over to you for re-review.

nfmohit

Excellent work here, @ankitrox!

I've left a small number of very minor comments. As they are very minor, I hope you won't mind if I go ahead and address them myself considering our time limitation.

assets/js/components/BadgeWithTooltip.js

assets/sass/components/global/_googlesitekit-badge-with-tooltip.scss

assets/sass/admin.scss

assets/sass/components/global/_googlesitekit-badge-with-tooltip.scss

.../analytics-4/components/audience-segmentation/dashboard/AudienceSelectionPanel/index.test.js

nfmohit

LGTM, brilliant work, @ankitrox !

Note: I've verified that the failing tests in CI are unrelated to this PR.

Add BadgeWithTooltip component.

3b5f67b

ankitrox added 11 commits September 9, 2024 13:08

Pass BadgeWithTooltip to SelectionPanelItem component.

948e8f1

Update VRT reference images.

04adcf2

Add badge prop support.

c182466

Add css.

f71b22c

Add story for temporarily hidden badge item.

ad7d252

Add VRT images.

aa36c7a

Merge branch 'develop' into enhancement/9096-add-temporarily-hidden-b…

de24ba5

…adge.

Add css for badges.

7d2383b

Resolve conflicts with develop.

24414f7

Add googlesitekit-badge class.

bb79bbb

Update VRT images.

8d1a73f

benbowler requested changes Sep 17, 2024

View reviewed changes

Update story for badge with tooltip.

4549ebc

benbowler approved these changes Sep 18, 2024

View reviewed changes

nfmohit requested changes Sep 19, 2024

View reviewed changes

nfmohit requested changes Sep 20, 2024

View reviewed changes

assets/sass/components/analytics-4/audience-segmentation/_googlesitekit-badge-with-tooltip.scss Outdated Show resolved Hide resolved

ankitrox added 2 commits September 20, 2024 18:39

Update tooltip text.

45684d2

Merge branch 'develop' into enhancement/9096-add-temporarily-hidden-b…

219ff36

…adge.

nfmohit requested changes Sep 20, 2024

View reviewed changes

techanvil reviewed Sep 20, 2024

View reviewed changes

...nalytics-4/components/audience-segmentation/dashboard/AudienceSelectionPanel/AudienceItem.js Outdated Show resolved Hide resolved

Fix tooltip text.

6663532

ankitrox added 2 commits September 24, 2024 20:09

Address CR feedback.

5c4cf39

Add tests for badge priorities.

198a787

nfmohit requested changes Sep 24, 2024

View reviewed changes

nfmohit added 2 commits September 25, 2024 01:55

Address minor CR feedback.

7de4a4d

Update VRT references.

8c01dd5

nfmohit approved these changes Sep 24, 2024

View reviewed changes

nfmohit merged commit f3f13f3 into develop Sep 24, 2024
17 of 21 checks passed

nfmohit deleted the enhancement/9096-add-temporarily-hidden-badge branch September 24, 2024 20:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ankitrox commented Sep 9, 2024 •

edited by nfmohit

Loading

github-actions bot commented Sep 9, 2024 •

edited

Loading

github-actions bot commented Sep 9, 2024 •

edited

Loading

benbowler left a comment

benbowler Sep 17, 2024

benbowler Sep 17, 2024

benbowler Sep 17, 2024

ankitrox commented Sep 18, 2024

benbowler left a comment

nfmohit left a comment

nfmohit left a comment

nfmohit left a comment •

edited

Loading

techanvil commented Sep 20, 2024

ankitrox commented Sep 24, 2024

nfmohit left a comment

nfmohit left a comment

	label: 'Modules/Analytics4/Components/BadgeWithTooltip/Default',
	label: 'Components/BadgeWithTooltip/Default',

	title: 'Modules/Analytics4/Components/BadgeWithTooltip',
	title: 'Components/BadgeWithTooltip',

	'Still collecting full data for this timeframe, partial data is displayed for this group',
	'This is an example of tooltip content for a badge with tooltip',

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

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

Conversation

ankitrox commented Sep 9, 2024 • edited by nfmohit Loading

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

github-actions bot commented Sep 9, 2024 • edited Loading

github-actions bot commented Sep 9, 2024 • edited Loading

benbowler left a comment

Choose a reason for hiding this comment

benbowler Sep 17, 2024

Choose a reason for hiding this comment

benbowler Sep 17, 2024

Choose a reason for hiding this comment

benbowler Sep 17, 2024

Choose a reason for hiding this comment

ankitrox commented Sep 18, 2024

benbowler left a comment

Choose a reason for hiding this comment

nfmohit left a comment

Choose a reason for hiding this comment

nfmohit left a comment

Choose a reason for hiding this comment

nfmohit left a comment • edited Loading

Choose a reason for hiding this comment

techanvil commented Sep 20, 2024

ankitrox commented Sep 24, 2024

nfmohit left a comment

Choose a reason for hiding this comment

nfmohit left a comment

Choose a reason for hiding this comment

ankitrox commented Sep 9, 2024 •

edited by nfmohit

Loading

github-actions bot commented Sep 9, 2024 •

edited

Loading

github-actions bot commented Sep 9, 2024 •

edited

Loading

nfmohit left a comment •

edited

Loading