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.22k stars 277 forks source link

Implement `<PublicationApprovedOverlayNotification>` component #8843

Open nfmohit opened 1 month ago

nfmohit commented 1 month ago

Feature Description

A <PublicationApprovedOverlayNotification> component should be implemented for the Reader Revenue Manager module that renders an <OverlayNotification> component in the Site Kit dashboard.

Screenshot for reference ![image](https://github.com/google/site-kit-wp/assets/20284937/d12a5458-cf8c-4fdb-b892-23e37882b9ce)

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. Make sure module is enabled in tester plugin.

  2. Activate the module and go to Site Kit > Dashboard. Do not do any configuration as of now.

  3. Run the following command in browser console.

googlesitekit.data.dispatch( 'core/ui' ).setValue( 'READER_REVENUE_MANAGER_SHOW_PUBLICATION_APPROVED_NOTIFICATION', true );
  1. As soon as above command is run, it should display the overlay notification.

  2. "Enable Features" should take to the publisher centre.

https://publishercenter.google.com/?utm_source=sitekit
  1. Clicking on "Maybe later" should dismiss the notification. Refreshing the page should not show the notification again.

Changelog entry

nfmohit commented 3 weeks ago

Thank you for the IB, @ankitrox ! Please take a look at my comments below:

  • Add RRM_PUBLICATION_APPROVED_OVERLAY_NOTIFICATION to assets/js/module/reader-revenue-manager/ with value rrmPublicationApprovedOverlayNotification.

We'd want to add this constant to the same component file and export it as well. For reference, see LINK_ANALYTICS_ADSENSE_OVERLAY_NOTIFICATION in assets/js/components/OverlayNotification/LinkAnalyticsAndAdSenseAccountsOverlayNotification.js.

  • In the component check if the component is dismissed, if it is dismissed, pass shouldShowNotification prop to OverlayNotification accordingly.

We should further expand on the conditions for the shouldShowNotification prop. Please add the following criteria:

  1. It should only show up if it is not already dismissed (this condition already exists in the current IB).
  2. It should only show up for a non-view-only user (see useViewOnly hook). This requirement wasn't added in the ACs and I just added it, sorry!
  3. It should only show up in the main dashboard (see useDashboardType hook). This requirement wasn't added in the ACs and I just added it, sorry!
  4. It should only show up if the core/ui store key mentioned in the AC is set to true. We also need to add that key as a constant somewhere like assets/js/modules/reader-revenue-manager/datastore/constants.js.

Finally, we want to render this component in assets/js/components/OverlayNotification/OverlayNotificationsRenderer.js, but only if the rrmModule feature flag is enabled.

  • Pass the different graphics for GraphicDesktop and GraphicMobile according to the figma designs.

The link to the Figma designs here points to a different mock. It is sufficient to mention that the Figma designs from the ACs should be referenced.

  • Enable features CTA should use Button component with href prop passed to it. The link should be https://publishercenter.google.com/reader-revenue-manager?publication={$publicationID} where the $publicationID is the approved publication ID.

Similar to other issues, the link should use the getServiceURL selector with the publication ID passed to it.


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

ankitrox commented 3 weeks ago

Thanks for the IB review @nfmohit . I've made the suggested changes in IB.

Assigning back to you for review.

nfmohit commented 3 weeks ago

Thank you for updating the IB, @ankitrox ! I've left a few follow-up comments:

  • Add and export constant RRM_PUBLICATION_APPROVED_OVERLAY_NOTIFICATION to assets/js/modules/reader-revenue-manager/datastore/constants.js with value rrmPublicationApprovedOverlayNotification.

Similar to other overlay notification components, I think this should reside in the component file itself, i.e. assets/js/module/reader-revenue-manager/components/PublicationApprovedOverlayNotification.js.

  • Rendering the banner/popup is handled here. It will be visible when the RRM feature is enabled.

I think this statement is no longer necessary as we're handling the rendering of the component in this issue.

  • RRM_PUBLICATION_APPROVED_OVERLAY_NOTIFICATION key from CORE_UI store is set to true.

This constant doesn't currently exist, so it should be added to assets/js/modules/reader-revenue-manager/datastore/constants.js as mentioned in the ACs.


Please let me know what you think, thank you!

ankitrox commented 3 weeks ago

Sorry @nfmohit , I was initially bit confused with both constants, but I've made it clear in IB now that one is for notification ID and other is for UI store.

Updated the IB according to suggestions.

Thank you.

nfmohit commented 3 weeks ago

Looks fantastic now, thank you @ankitrox ! IB LGTM 👍 ✅

ankitrox commented 1 week ago

Hi @kuasha420 ,

If you haven't started with this one, just a heads up that UI_KEY_SHOW_RRM_PUBLICATION_APPROVED_NOTIFICATION constant has been added in #8796 here

Thanks