Allow notifications to be registered and immediately shown #10782
Conversation
|
Size Change: +822 B (+0.04%) Total Size: 2.08 MB ℹ️ View Unchanged
|
|
Build files for 4b8a565 are ready:
|
| for ( const viewContext of viewContexts ) { | ||
| yield { | ||
| type: POPULATE_QUEUE, | ||
| payload: { | ||
| viewContext, | ||
| groupID, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
@tofumatt There are a few problems with populating the queue here
- The
queuedNotificationsstate is only segmented bygroupID. The queues are intended to be populated for theviewContextwhere we are requesting notifications for from the selector. By calling this for all contexts the notification supports, we will effectively populate the queues with all notifications across all viewContexts which would result in theviewContextbeing ignored as queue groups would be populated with all notifications for that group. - Populating the queue at registration time will undermine the way it is ordered, which is intended to run with the full list of registered notifications in order to ensure all of the priorities are considered. Thinking of it iteratively, when we register the first notification, it will populate the queue, and will queue the one and only notification. The same will happen essentially on every subsequent invocation since there will by definition only be one notification that has not been queued yet, so priorities will no longer be considered. We could update it to sort by priority as well, but that would nullify the priority group racing which prioritizes notifications (in the same priority group) which are ready first.
- Populating the queue on registration will cause
checkRequirementsto be evaluated in a pre-loaded way rather than on-demand as they should be which could result in some API requests being made too often or when not needed.
The good news though is that I think the solution here is simpler. We should preserve the selector->resolver-based queue population since this is where the current queue is cleared and repopulated on-demand. The only thing we really need to change here is to invalidate the resolver (which can be done for all args in one go) instead. This way, the queue will still be populated at the same time it is today and there is no penalty for invalidating an already invalidated resolver which makes it much less impactful than populating.
The only drawback/challenge I can think of is that this could result in a different queue when repopulated in this way than it had before which would be a poor experience for the user. Perhaps the solution here is to not clear the queues unless the viewContext has changed if already populated. Then when populating the queue, we'd filter out all notifications that are already queued from being re-evaluated for population, but this would mean on-demand notifications would always go to the end of the queue where they may not be seen at all.
Alternatively, we could still clear the queue safely, so long as we preserve the head of each group – that way we'd never be changing the "current"/active notification which is important to avoid disrupting the UX.
Lastly, we need to make sure that runtime side-effects such as marking a notification as "viewed" don't cause it to be rotated out due to changes we're making here that involve re-evaluating the queues.
It sounds like we're running into many of the same challenges we initially identified in the comments on #9453 but I feel like we should be able to make it work by addressing the above. Ultimately, in order to to really have an on-demand notification that will be shown right away that doesn't break existing queues is to decouple it from the existing location (avoid flicker/layout-shift) and groups (avoid disrupting queues/order). Eventually, I think we'll want/need a new N area + N group for snackbar type notifications that float in the corner and allows for multiple to be shown simultaneously. These should be used conservatively but are also an established pattern (i.e. in Gmail when you archive/delete an email). This is out of scope for this issue but something I believe we are aiming for in the future that should compliment the work here rather than invalidate it.
cc: @jimmymadon
There was a problem hiding this comment.
Invalidating the resolver for getQueuedNotifications will cause the already displayed/loaded notifications to be removed (eg. "disappear") for a split-second while they're reloaded though, because resetQueue is called before the queue is repopulated. I wonder if we should refrain from resetting the queue anytime we reload the notifications to prevent any UI bugs here 🤔
| checkRequirements: () => { | ||
| return true; | ||
| }, |
There was a problem hiding this comment.
checkRequirements is optional and will be treated the same way if not provided
site-kit-wp/assets/js/googlesitekit/notifications/datastore/notifications.js
Lines 377 to 382 in 1f927c2
tofumatt
changed the title
Summary
Addresses issue:
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist