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.23k stars 279 forks source link

Refactor the PAXSetupSuccessSubtleNotification to use the new Notifications datastore #8980

Closed jimmymadon closed 1 week ago

jimmymadon commented 2 months ago

Feature Description

This is the second issue that refactors another "subtle notifications" in the plugin as part of Phase 1 of the Banner Notifications Refactoring epic. It should refactor the PAXSetupSuccessSubtleNotification 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

Implementation Brief

Test Coverage

QA Brief

Changelog entry

tofumatt commented 1 month ago

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.

eugene-manuilov commented 1 month ago

AC ✔️

eugene-manuilov commented 3 weeks ago
  • In assets/js/googlesitekit/notifications/datastore/notifications.js:
    • Within the getQueuedNotifications resolver, fetch the notification and slug using getQueryArg() and pass these to the checkRequirements() callback function (alongwith viewContext).

@jimmymadon, I think we shouldn't put it into the getQueuedNotifications resolver because it is very specific thing for just one notifications (or few notifications). Let's pull that info in the checkRequirements function of the notification itself.

Also, no need to use checkboxes for every subitem in IB, just use it for top level items or for subitems if we have 3+ levels of nested items. Finally, the 19 estimates seems a bit big, do we really need a half of the week to implement that? Seems like an over estimate because we need to refactor the existing component, not to create it from scratch.

eugene-manuilov commented 3 weeks ago

IB ✔️

mohitwp commented 1 week ago

QA Update ⚠️

@jimmymadon I noticed that when the user clicks on 'Show me,' the page scrolls down to the Campaign Performance widget, but the scroll position is slightly off. Should this issue be fixed in a follow-up, or should I create a separate ticket for it?

https://github.com/user-attachments/assets/bc12472f-648e-4da7-9fb3-308d03fbcac1

PASS CASES

![image](https://github.com/user-attachments/assets/4e52a7e8-bd6f-4bf0-8fc1-8850b78c82a5) Dismiss event ![image](https://github.com/user-attachments/assets/3e285947-da8e-4c49-9db6-d659ae420dc0)
tofumatt commented 1 week ago

@jimmymadon I noticed that when the user clicks on 'Show me,' the page scrolls down to the Campaign Performance widget, but the scroll position is slightly off. Should this issue be fixed in a follow-up, or should I create a separate ticket for it?

That would be separate/unrelated to the notification refactoring done here, can you please file a separate issue for that? 🙏🏻

mohitwp commented 1 week ago

QA Update ✅

Note : There is a issue with zero data notification so it do not appear immediately once Ads pax success notice gets dismiss.

![image](https://github.com/user-attachments/assets/4e52a7e8-bd6f-4bf0-8fc1-8850b78c82a5) **Dismiss event** ![image](https://github.com/user-attachments/assets/3e285947-da8e-4c49-9db6-d659ae420dc0) https://github.com/user-attachments/assets/1ae372a8-dee5-4737-ae56-99437c9dc53a https://github.com/user-attachments/assets/15b2c1e3-e7a9-4660-ab7c-32d8d70a6d69