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 287 forks source link

Limit all banners shown in the header to only one visible at a time and consolidate placement #6634

Closed aaemnnosttv closed 11 months ago

aaemnnosttv commented 1 year ago

Feature Description

In #4689, we updated our handling of notifications to prevent showing more than one notice in the header at a time, even when multiple are present. However, this only applies to each group of notifications rather than all, and since there are separate placements for persistent alerts and notices, there can be up to two notices showing simultaneously.


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

aaemnnosttv commented 1 year ago

@marrrmarrr let me know if this sounds like what we discussed today or if there are any changes you would like to make to the AC 👍

marrrmarrr commented 1 year ago

@aaemnnosttv thanks for creating this issue! The only comment I have is if we need to add an extra instruction for when two higher-severity alerts are somehow triggered by the same site (you mentioned there's 3-4 of them altogether, not sure if it would be ever possible that the same site has two?) If that is possible, how about we show the one which was triggered earlier? And once the user resolves that one, then show the next one.

aaemnnosttv commented 1 year ago

@marrrmarrr it is possible multiple notifications/alerts could be triggered simultaneously which is somewhat what this issue is about. Currently we have a rather simple way of limiting the display which just shows the first one. When that one is dismissed/actioned, it will disappear causing the next one to appear if there is one. In this case, priority is set ahead of time in the order the elements are rendered, which is not the same as which is triggered first. So currently it is actually possible for a higher priority alert to appear after you see another notice if there was some amount of latency which prevented it from "knowing" that it should be shown or not until later in the page load. This is pretty rare but possible. For the most part, everything we need to determine if a notification should be shown or not is available nearly right away so it's rarely an issue.

Solving this requires rearchitecting the way we handle notifications (which will need to be done for the new Notifications work anyways) so it's out of scope here, but is something we can work on in preparation for the other notifications-related work we'll have.

aaemnnosttv commented 1 year ago

Moved forward after discussing together earlier today 👍

eugene-manuilov commented 11 months ago

IB ✔️

wpdarren commented 11 months ago

QA Update: ✅

Verified:

Screenshots ![image](https://github.com/google/site-kit-wp/assets/73545194/c2dc55bf-5ba3-4b43-b021-a47c92340fc4) ![image](https://github.com/google/site-kit-wp/assets/73545194/1890459c-39d5-49b3-ae67-076d854a2158)