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 Banner Notifications with the Admin Menu tooltip on Dismissal #10003

Open
1 task
jimmymadon opened this issue Jan 9, 2025 · 1 comment
Open
1 task
Assignees
Labels
Next Up Issues to prioritize for definition P1 Medium priority Team S Issues for Squad 1 Type: Enhancement Improvement of an existing feature

Comments

@jimmymadon
Copy link
Collaborator

jimmymadon commented Jan 9, 2025

Feature Description

The FPM, RRM, Consent Mode and Audience Segmentation Setup CTA banners all follow a similar pattern. When the Maybe later dismiss CTA is clicked, the banner disappears and shows an <AdminMenuTooltip>. Currently, the main notification component (e.g. ReaderRevenueManagerSetupCTABanner) also conditionally renders the tooltip component. This causes a few issues:

  1. We cannot use the new automatic dismissal infrastructure added in the BNR epic. If we do, it changes the state of the queuedNotifications and removes the whole component from the React component tree, including the visible tooltip.
  2. Currently, we skipHidingFromQueue on dismissal, which means the queuedNotifications do not update a preventing other notifications in the queue to render next. The dismissed notification is still hanging around in the component tree even after the tool tip is dismissed and the AdminTooltip component is removed from the tree.

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

Acceptance criteria

[Please see Note to AC / IB reviewer below]

  • For the RRM, FPM, Consent mode and Audience Segmentation setup banners that have been newly refactored, when any of these banners are dismissed by clicking on the Maybe later button, the Admin Menu tooltip should be made visible. And if there is another Setup CTA in the queue, it should appear next without having to reload the page.
  • The 'hack' which sets skipHidingFromQueue to true on dismissal, preventing the dismissal of the notification should be removed.
  • Any code that hides the notification component (return null) when the notification is dismissed should be removed.

Implementation Brief

  • In `

Test Coverage

QA Brief

Changelog entry

@jimmymadon
Copy link
Collaborator Author

Note to AC Reviewer:

Currently, the next-in-line Setup Banner CTA does not immediately render after a banners dismissal. The tooltip shows but no other setup CTA is rendered, even if it is "ready and eligible". This "bug" is indirectly good UX for now! When the user reloads the dashboard, the next in line notification will show then. However, this behaviour is non-standard for our new notifications infrastructure and needs to be "fixed". The ACs here suggests this, even if immediately showing the next in line banner is "bad UX". The scheduling of banners and having a "cool down" period is being separately addressed in #10019.

Note to IB Reviewer:

I've outlined 2 vague approaches, both of which aren't super convincing. Perhaps I am missing a simpler pattern that we could be following here?

@jimmymadon jimmymadon removed their assignment Jan 29, 2025
@jimmymadon jimmymadon added P1 Medium priority Type: Enhancement Improvement of an existing feature Team S Issues for Squad 1 Next Up Issues to prioritize for definition labels Jan 29, 2025
@jimmymadon jimmymadon self-assigned this Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Next Up Issues to prioritize for definition P1 Medium priority Team S Issues for Squad 1 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

1 participant