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 AdSenseAlerts to use the new Notifications datastore #9300

Open
1 task
zutigrm opened this issue Sep 4, 2024 · 5 comments
Open
1 task

Refactor AdSenseAlerts to use the new Notifications datastore #9300

zutigrm opened this issue Sep 4, 2024 · 5 comments
Assignees
Labels
P2 Low priority Team S Issues for Squad 1 Type: Enhancement Improvement of an existing feature

Comments

@zutigrm
Copy link
Collaborator

zutigrm commented Sep 4, 2024

Feature Description

This issue should refactor the AdSenseAlerts so that it uses the new datastore infrastructure to register and queue the notification. It should also incorporate a new "layout".


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

Acceptance criteria

  • The AdSenseAlerts 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. in BannerNotifications) 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.
  • The component should not use the bloated BannerNotification component. Instead, it should be rendered using the new Notification component wrapper and a simpler "layout" component that solely encapsulates its structure and design.

Implementation Brief

Test Coverage

QA Brief

Changelog entry

@zutigrm zutigrm added P2 Low priority Type: Enhancement Improvement of an existing feature Team S Issues for Squad 1 labels Sep 4, 2024
@zutigrm zutigrm assigned zutigrm and unassigned zutigrm Sep 4, 2024
@eugene-manuilov eugene-manuilov self-assigned this Sep 5, 2024
@eugene-manuilov
Copy link
Collaborator

AC ✔️

@eugene-manuilov eugene-manuilov removed their assignment Sep 5, 2024
@zutigrm zutigrm self-assigned this Sep 6, 2024
@zutigrm
Copy link
Collaborator Author

zutigrm commented Oct 3, 2024

I am un-assigning myself since I have been caught up with higher priority epic issues, and will be on the PTO for few days. This notification will take a bit of a thinking it over, as it is potentially a group of notifications that are fetched from the backend, and current notifications API might need some tweaks to support this. The idea I had when initially had time to check this one, is that regular approach would potentially work, as we would register the notification in the API, and then checkRequirements and notification component would handle the fetching and iteration over data. Something to consider was that isDismissable is a fixed property included when notification is registered in the API, if the notifications coming from the backend are different in that regard, it won't render correctly, since isDismissable would be applied to all with the same value either true or false

@benbowler
Copy link
Collaborator

I'm marking this blocked by #9294 as there is an ongoing discussion there regarding the module level createNotificationsStore. This in an example of notifications that use this existing infrastructure so we need a unified spec/decision on how we want to approach this existing notifications store to bring it into the new store.

@benbowler benbowler removed their assignment Nov 5, 2024
@jimmymadon jimmymadon self-assigned this Feb 10, 2025
@benbowler
Copy link
Collaborator

Note around the priority of these banners, AdSense banners are the only banners which should be higher priority than ModuleRecoveryAlert which I have set in #10398, so in this ticket we should set it to higher priority (lower number) and other refactored BannerNotifications should be lower priority (higher number).

@jimmymadon
Copy link
Collaborator

Having discussed this and #9294 with @aaemnnosttv, we decided to hold off on these issues to prevent "nested" queues of notifications. This is because these notification components call a getNotifications selector which fetches one or more notifications and maps through them to render them. We might be better off creating these as "on-demand" notifications and so, we will wait till #9453 is completed first before we amend the AC/IB here.

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

4 participants