site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard
verified publisher icon google

Implement `<PublicationApprovedOverlayNotification>` component

Open nfmohit opened this issue 1 year ago • 5 comments

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

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

Acceptance criteria

  • A <PublicationApprovedOverlayNotification> component should be added for the Reader Revenue Manager module according to the Figma designs that renders an overlay notification that lets the user know that their publication was approved.
  • Its heading should be "Your Reader Revenue Manager publication is approved".
  • Its description should be "Unlock your full reader opportunity by enabling features like subscriptions, contributions and newsletter sign ups".
  • Its dismissal, "Maybe later", should dismiss the notification persistently.
  • Its CTA, "Enable features" (with an external link icon), should open the publication in Reader Revenue Manager in a new browser tab.
  • It should be rendered in the Site Kit dashboard only if the rrmModule feature flag is enabled AND a key in the core/ui store is set to true, e.g. show-rrm-publication-approved-notification (with constant UI_KEY_SHOW_RRM_PUBLICATION_APPROVED_NOTIFICATION).
  • It should adhere to the mobile designs in Figma here.

Implementation Brief

  • [ ] Add a constant UI_KEY_SHOW_RRM_PUBLICATION_APPROVED_NOTIFICATION to assets/js/modules/reader-revenue-manager/datastore/constants.js with value 'show-rrm-publication-approved-notification'.
  • [ ] Create assets/js/module/reader-revenue-manager/components/PublicationApprovedOverlayNotification.js
    • [ ] Add and export constant RRM_PUBLICATION_APPROVED_OVERLAY_NOTIFICATION with value rrmPublicationApprovedOverlayNotification. This will be passed as notificationID to OverlayNotification component.
    • [ ] In the component, create the shouldShowNotification according to following points. This prop will be passed to OverlayNotification. Note that this should fulfill all of the following conditions to be true.
      • [ ] Not dismissed already. Use RRM_PUBLICATION_APPROVED_OVERLAY_NOTIFICATION to check this.
      • [ ] User is non-view-only user (use useViewOnly hook).
      • [ ] Dashboard is the main dashboard (use useDashboardType hook).
      • [ ] UI_KEY_SHOW_RRM_PUBLICATION_APPROVED_NOTIFICATION key from CORE_UI store is set to true. This will be set to true by syncPublicationOnboardingState action (refer #8796).
    • [ ] Pass the different graphics for GraphicDesktop and GraphicMobile according to the figma designs.
    • [ ] Also note that button positions are different for desktop and mobile. We can use useBreakpoint hook do position the CTAs.
    • [ ] The heading and description should be copied from the AC.
    • [ ] Create a function dismissNotification in component
    • [ ] Clicking on Maybe later should called dismissOverlayNotification from CORE_UI store. Pass RRM_PUBLICATION_APPROVED_OVERLAY_NOTIFICATION to dismissOverlayNotification.
    • [ ] Enable features CTA should use Button component with href prop passed to it. The link should be be passed using getServiceURL selector (#8848), pass publication ID to the selector.
    • [ ] Both the CTAs should be in disabled state when the notice is being dismissed. It can be checked using isDismissingItem selector from CORE_USER store.
  • [ ] In assets/js/components/OverlayNotification/OverlayNotificationsRenderer.js, render rrmPublicationApprovedOverlayNotification if RRM feature is enabled (refer useFeature hook).

Test Coverage

  • Add tests for PublicationApprovedOverlayNotification, also test dismissal behaviour.
  • Add story for PublicationApprovedOverlayNotification component.

QA Brief

Changelog entry

nfmohit avatar Jun 08 '24 11:06 nfmohit

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!

nfmohit avatar Jul 04 '24 03:07 nfmohit

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

Assigning back to you for review.

ankitrox avatar Jul 04 '24 04:07 ankitrox

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!

nfmohit avatar Jul 04 '24 06:07 nfmohit

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.

ankitrox avatar Jul 04 '24 07:07 ankitrox

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

nfmohit avatar Jul 04 '24 07:07 nfmohit

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

ankitrox avatar Jul 15 '24 14:07 ankitrox

I've asked Sigal to help us with the SVG issue. All other comments have been addressed. Keeping this ticket here till we hear back from Sigal about SVG.

ankitrox avatar Jul 29 '24 10:07 ankitrox

QA Update: ⚠️

@nfmohit I have two observations with questions.

  1. When I click on the Enable features CTA, a new window opens and I am redirected to the publisher center. When I come back to the dashboard though, the overlay notification still appears. Based on my experience with other overlay notifications, when we complete an action, it would usually disappear. What's the expectation here?
  2. The URL I feel is right but I don't want to make any assumptions. I see https://publishercenter.google.com/u/2/?utm_source=sitekit&pli=1&publication=CAowq-qvDA so there are some additional parts to the URL when compared with the QAB. Can you confirm this is expected?

https://github.com/user-attachments/assets/d60c58e0-ba1d-47d7-a810-6254c65ad932

wpdarren avatar Aug 01 '24 12:08 wpdarren

Thank you for sharing your observations, @wpdarren !

  1. When I click on the Enable features CTA, a new window opens and I am redirected to the publisher center. When I come back to the dashboard though, the overlay notification still appears. Based on my experience with other overlay notifications, when we complete an action, it would usually disappear. What's the expectation here?

Hmm, while this wasn't a part of the ACs, now that I take a look at the behaviour, it appears that dismissing the notification after the publisher center is opened in a new tab will be the appropriate behaviour. I'll open a quick follow-up PR to address this.

2. The URL I feel is right but I don't want to make any assumptions. I see https://publishercenter.google.com/u/2/?utm_source=sitekit&pli=1&publication=CAowq-qvDA so there are some additional parts to the URL when compared with the QAB. Can you confirm this is expected?

The URL is correct. The rest of the parts are actually added by Publisher Center themselves.

nfmohit avatar Aug 02 '24 04:08 nfmohit

Ankit was very kind to open the follow-up PR, thanks @ankitrox! Back to you for another pass, @wpdarren.

nfmohit avatar Aug 02 '24 22:08 nfmohit

QA Update: ✅

Verified:

  • A <PublicationApprovedOverlayNotification> component is added for the Reader Revenue Manager module according to the Figma designs as an overlay notification that lets the user know that their publication was approved.
  • Its heading is Your Reader Revenue Manager publication is approved
  • Its description is Unlock your full reader opportunity by enabling features like subscriptions, contributions and newsletter sign ups
  • Its dismissal, Maybe later, dismisses the notification persistently. Refreshed numerous times, cleared cache etc.
  • Its CTA, Enable features opens the publication in Reader Revenue Manager in a new browser tab.
  • It is rendered in the Site Kit dashboard only if the rrmModule feature flag is enabled AND a key in the core/ui store.
  • It adheres to the mobile designs in Figma here.
  • The issue reported above about the behavioiur of the overlay notification when you click the CTA has been fixed.
Screenshots

rrm-1 image

wpdarren avatar Aug 05 '24 05:08 wpdarren