RRM Setup Success Notification #9117

kuasha420 · 2024-08-02T05:07:51Z

Summary

Addresses issue:

Implement RRM module setup success banner #8840

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.

…ot reliable for query params.

github-actions · 2024-08-06T03:42:40Z

Build files for da3ec15 have been deleted.

hussain-t

Great work, @kuasha420. The changes almost look good. However, I've left a few comments. Please take a look at them.

assets/js/components/notifications/SubtleNotification.js

assets/js/components/notifications/SubtleNotification.stories.js

hussain-t · 2024-08-06T10:33:30Z

assets/js/components/notifications/SubtleNotification.stories.js

+};
+WarningWithExternalCTA.scenario = {};
+
+export default {


Can we add a story that passes a custom Icon prop?

assets/js/components/notifications/SubtleNotification.stories.js

hussain-t · 2024-08-06T11:16:27Z

.../js/modules/reader-revenue-manager/components/dashboard/RRMSetupSuccessSubtleNotification.js

+				) }
+				onDismiss={ onDismiss }
+				dismissLabel={ __( 'Maybe later', 'google-site-kit' ) }
+				ctaLink={ serviceURL }


Do we need to dismiss the notification when clicking the CTA? cc: @nfmohit

I think so, but I'll let @nfmohit to confirm.

If so, we can simply pass the onDismiss callback to the onCTAClick prop.

Hmm, I think that would make sense. Let's do it.

Thanks for confirming, @nfmohit.

hussain-t · 2024-08-06T14:24:08Z

@kuasha420, it would be nice to have unit test coverage for the RRMSetupSuccessSubtleNotification component.

github-actions · 2024-08-07T08:21:22Z

Size Change: +20.7 kB (+1.22%)

Total Size: 1.72 MB

Filename	Size	Change
`./dist/assets/css/googlesitekit-admin-css-********************.min.css`	57 kB	+240 B (+0.42%)
`./dist/assets/js/36-********************.js`	3.12 kB	-1 B (-0.03%)
`./dist/assets/js/googlesitekit-activation-********************.js`	23.8 kB	+85 B (+0.36%)
`./dist/assets/js/googlesitekit-ad-blocking-recovery-********************.js`	63.6 kB	+3.77 kB (+6.3%)	🔍
`./dist/assets/js/googlesitekit-adminbar-********************.js`	34.6 kB	+33 B (+0.1%)
`./dist/assets/js/googlesitekit-api-********************.js`	10 kB	+1 B (+0.01%)
`./dist/assets/js/googlesitekit-components-gm2-********************.js`	5.97 kB	+6 B (+0.1%)
`./dist/assets/js/googlesitekit-components-gm3-********************.js`	10.1 kB	+3 B (+0.03%)
`./dist/assets/js/googlesitekit-data-********************.js`	2.35 kB	-1 B (-0.04%)
`./dist/assets/js/googlesitekit-datastore-forms-********************.js`	9.03 kB	-1 B (-0.01%)
`./dist/assets/js/googlesitekit-datastore-site-********************.js`	20.4 kB	-4 B (-0.02%)
`./dist/assets/js/googlesitekit-datastore-user-********************.js`	25.3 kB	+5 B (+0.02%)
`./dist/assets/js/googlesitekit-entity-dashboard-********************.js`	78.9 kB	-389 B (-0.49%)
`./dist/assets/js/googlesitekit-main-dashboard-********************.js`	153 kB	+1.3 kB (+0.86%)
`./dist/assets/js/googlesitekit-modules-********************.js`	22.3 kB	+165 B (+0.74%)
`./dist/assets/js/googlesitekit-modules-ads-********************.js`	29.6 kB	+83 B (+0.28%)
`./dist/assets/js/googlesitekit-modules-adsense-********************.js`	117 kB	+3.48 kB (+3.06%)
`./dist/assets/js/googlesitekit-modules-analytics-4-********************.js`	165 kB	-30 B (-0.02%)
`./dist/assets/js/googlesitekit-modules-pagespeed-insights-********************.js`	22.7 kB	+89 B (+0.39%)
`./dist/assets/js/googlesitekit-modules-reader-revenue-manager-********************.js`	20.5 kB	-8 B (-0.04%)
`./dist/assets/js/googlesitekit-modules-search-console-********************.js`	58.6 kB	-321 B (-0.54%)
`./dist/assets/js/googlesitekit-modules-tagmanager-********************.js`	32.1 kB	+19 B (+0.06%)
`./dist/assets/js/googlesitekit-notifications-********************.js`	4.04 kB	-2 B (-0.05%)
`./dist/assets/js/googlesitekit-polyfills-********************.js`	376 B	-1 B (-0.27%)
`./dist/assets/js/googlesitekit-settings-********************.js`	77.8 kB	+3.42 kB (+4.6%)	🔍
`./dist/assets/js/googlesitekit-splash-********************.js`	78.4 kB	+4.3 kB (+5.8%)	🔍
`./dist/assets/js/googlesitekit-user-input-********************.js`	53.2 kB	+4.35 kB (+8.91%)	🔍
`./dist/assets/js/googlesitekit-vendor-********************.js`	321 kB	+54 B (+0.02%)
`./dist/assets/js/googlesitekit-widgets-********************.js`	81.7 kB	+65 B (+0.08%)
`./dist/assets/js/googlesitekit-wp-dashboard-********************.js`	61.8 kB	+21 B (+0.03%)
`./dist/assets/js/runtime-********************.js`	1.3 kB	+1 B (+0.08%)

ℹ️ 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/35-********************.js`	892 B
`./dist/assets/js/analytics-advanced-tracking-********************.js`	901 B
`./dist/assets/js/googlesitekit-consent-mode-********************.js`	25.6 kB
`./dist/assets/js/googlesitekit-datastore-location-********************.js`	2.08 kB
`./dist/assets/js/googlesitekit-datastore-ui-********************.js`	9.98 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}

kuasha420 · 2024-08-07T09:26:19Z

@hussain-t Thank you for your thorough code review on this. I've address all of them. Please, have a look.

I tried to add tests for the RRMSetupSuccessSubtleNotification but faced some issues with the query args, hence I added the VRTs for it instead as discussed internally.

Final note, the VRT is failing on completely unrelated components, and are passing locally.

Thank you.

.../js/modules/reader-revenue-manager/components/dashboard/RRMSetupSuccessSubtleNotification.js

...les/reader-revenue-manager/components/dashboard/RRMSetupSuccessSubtleNotification.stories.js

hussain-t

Thanks for addressing the feedback, @kuasha420. I've made a few minor changes, as noted in my comments. Besides that, after pulling the latest develop, I noticed some inconsistencies in the Storybook stories folder structure:

ReaderRevenueManager/Dashboard/RRMSetupSuccessSubtleNotification
ReaderRevenueManager/Components/Dashboard/ReaderRevenueManagerSetupCTABanner

We should maintain a consistent structure for the dashboard components. I'm fine with either of the above options, so I'll leave the decision to you.

Additionally, @nfmohit suggested dismissing the notification when clicking the CTA. Let's incorporate this change as well. We can pass the onDismiss callback to the onCTAClick prop.

Thanks!

hussain-t

Excellent work, @kuasha420. LGTM 👍

So that you know, I've slightly updated the QAB.

Note: As discussed on Slack, let's create a new ticket to implement unit tests and reuse this components/notifications/SubtleNotification.js component instead of modules/analytics-4/components/SubtleNotification.js.

kuasha420 added 7 commits August 2, 2024 03:17

Add SubtleNotification component.

b281a6d

Add RRMSetupSuccessSubtleNotification component.

5639498

Add stories for RRMSetupSuccessSubtleNotification.

fb90d96

Register SetupSuccessSubtleNotification.

1f79b83

Merge branch 'develop' into enhance/8840-rrm-success-notification.

6f1201e

Add VRT images for SubtleNotification component.

346ebc9

Add VRT for RRM Success Notification stories as adding jest test is n…

73038b6

…ot reliable for query params.

kuasha420 marked this pull request as ready for review August 6, 2024 03:36

hussain-t requested changes Aug 6, 2024

View reviewed changes

kuasha420 added 3 commits August 7, 2024 12:59

Update component based on CR feedback.

92f0b51

Update stories based on CR feedback.

852ab1d

Update VRT images.

cb90656

kuasha420 requested a review from hussain-t August 7, 2024 09:26

hussain-t added 3 commits August 7, 2024 16:38

Pass onDismiss prop to fix console warning in stories.

eb99960

Merge branch 'develop' into enhance/8840-rrm-success-notification.

4183a07

User RRm module slug constant instead of a string.

6791bf7