Skip to content
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

Refactor RRMSetupSuccessSubtleNotification to use the new Notifications datastore #8981

Open
3 tasks done
jimmymadon opened this issue Jul 8, 2024 · 5 comments
Open
3 tasks done
Assignees
Labels
P2 Low priority Team S Issues for Squad 1 Type: Enhancement Improvement of an existing feature

Comments

@jimmymadon
Copy link
Collaborator

jimmymadon commented Jul 8, 2024

Feature Description

This is the first issue that refactors the newly implement "subtle notifications" in the plugin as part of Phase 1 of the Banner Notifications Refactoring epic. It should refactor the RRMSetupSuccessSubtleNotification so that it uses the new datastore infrastructure to register and queue the notification. It should also incorporate the newly introduced SubtleNotification component as a new "layout"


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

Acceptance criteria

  • The RRMSetupSuccessSubtleNotification component should be refactored so that it is registered and rendered (queued) using the new core/notifications datastore.
  • This notification component should not be called directly (i.e. directly in <SubtleNotifications>) but only via the generic getQueuedNotifications selector.
  • The refactored component should not contain any business logic that hides it / prevents it from rendering. This logic should be contained in a callback function defined during registration.
  • It should make use of the newly introduced SubtleNotification component and refactor this reusable component as a new "layout" within the new notifications framework.

Implementation Brief

  • Update assets/js/modules/reader-revenue-manager/components/dashboard/RRMSetupSuccessSubtleNotification.js
    • Check the assets/js/components/notifications/GatheringDataNotification.js for the example
    • Remove usage of SubtleNotification component
    • Include 2 new props - id and Notification
    • Wrap the component with Notification component passed as the prop
    • Refactor the existing props used for SubtleNotification to create a new layout assets/js/googlesitekit/notifications/components/layout/SubtleNotification.js
  • Update assets/js/googlesitekit/notifications/register-defaults.js
    • Register the notification, following the existing patterns already added
    • For checkRequirements transfer the existing checks from assets/js/components/notifications/SubtleNotifications.js and RRMSetupSuccessSubtleNotification component itself
    • For priority, bump it by 10 from the last added notification (not including error ones, which start from 150).
    • Use NOTIFICATION_AREAS.BANNERS_BELOW_NAV for areaSlug
  • Remove RRMSetupSuccessSubtleNotification from the assets/js/components/notifications/SubtleNotifications.js

Test Coverage

  • Update existing stories in assets/js/modules/reader-revenue-manager/components/dashboard/RRMSetupSuccessSubtleNotification.stories.js

QA Brief

  • Test the RRM Setup Success Notification once thoroughly by really completing RRM setup.
  • For additional retesting, you can use the following URL to trigger the notification: /wp-admin/admin.php?page=googlesitekit-dashboard&notification=authentication_success&slug=reader-revenue-manager
  • Test that all GA events include the "Publication Onboarding State" as the event label.
  • Test clicking on the main CTA which is an external link.
  • Test the dismiss behavior and new GA events being fired on dismissal.
  • Also test this notification when the site is in Gathering, Zero data and Unsatisfied Scope alert test. This notification will appear "first" and all others will appear later once this is dismissed.

Changelog entry

@jimmymadon jimmymadon added Type: Enhancement Improvement of an existing feature Team S Issues for Squad 1 labels Jul 8, 2024
@binnieshah binnieshah added the Next Up Issues to prioritize for definition label Jul 8, 2024
@eclarke1 eclarke1 added the P2 Low priority label Jul 8, 2024
@tofumatt tofumatt self-assigned this Jul 23, 2024
@tofumatt tofumatt changed the title Refactor the SetupSuccessSubtleNotification to use the new Notifications datastore Refactor SetupSuccessSubtleNotification to use the new Notifications datastore Jul 23, 2024
@tofumatt tofumatt removed their assignment Jul 23, 2024
@tofumatt
Copy link
Collaborator

Moving this to the backlog until #8976, which is marked as blocking this issue, is completed. That way we can mirror the approach used in #8976 for other migrations/refactoring.

@tofumatt tofumatt removed the Next Up Issues to prioritize for definition label Jul 23, 2024
@jimmymadon jimmymadon assigned jimmymadon and unassigned jimmymadon Aug 12, 2024
@jimmymadon jimmymadon changed the title Refactor SetupSuccessSubtleNotification to use the new Notifications datastore Refactor RRMSetupSuccessSubtleNotification to use the new Notifications datastore Aug 12, 2024
@eugene-manuilov eugene-manuilov self-assigned this Aug 13, 2024
@eugene-manuilov
Copy link
Collaborator

AC ✔️

@eugene-manuilov eugene-manuilov removed their assignment Aug 13, 2024
@zutigrm zutigrm assigned zutigrm and unassigned zutigrm Aug 16, 2024
@eugene-manuilov eugene-manuilov self-assigned this Aug 19, 2024
@eugene-manuilov
Copy link
Collaborator

IB ✔️

@jimmymadon
Copy link
Collaborator Author

@aaemnnosttv @sigal-teller

Screenshot 2024-09-23 at 13 42 54

When refactoring this issue, I noticed that when we click on the main CTA, it takes us to an external link in a new window but this does not dismiss the notification. Sometimes, this behaviour is actually good because it allows the user to visit the link again if they accidentally close the new window. So we should either:

  1. Change the "Maybe later" dismiss button on all version of this notification to be "Ok, got it!" so that if the user comes back from the external link window, then they can click on this button which makes more sense than "Maybe later".

OR

  1. Dismiss the notification on the main CTA click.

@sigal-teller
Copy link
Collaborator

@jimmymadon I was thinking about the first option as well. I think it's good idea to keep the CTA because if the user didn't complete customization or lost the page, it will be easier for him to re-visit the link and dismiss it at his convinience. I would replace the "Maybe later" with "got it", this is what we're using in otehr features.

@jimmymadon jimmymadon assigned nfmohit and unassigned jimmymadon Sep 23, 2024
@nfmohit nfmohit assigned jimmymadon and unassigned nfmohit Sep 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 Low priority Team S Issues for Squad 1 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

8 participants