Add the ability to pin notifications #11394
Conversation
…t a pinned notification at the top.
There was a problem hiding this comment.
Hey @nfmohit, thanks for working on this.
Unfortunately though, in my testing it's not working as expected. I'm seeing this error when returning to the dashboard from OAuth:
Here's a screencast:
10890-error-returning-from-oauth.webm
Digging in, it looks like snapshotting is stripping the Component, check, and checkRequirements functions out of the state.
Here I've console logged the queuedNotifications array before the snapshot:
And after the snapshot:
This is something we overlooked at the IB stage. It's not possible to serialise functions to JSON, so the CORE_NOTIFICATIONS datastore is not a good candidate for snapshotting. In fact, it's not good practice to store functions in state at all, for this reason. We should ideally refactor our state management to avoid this, but this is something to address separately - I'll add an issue to look into this.
In the meantime, we'll need to change the approach for this enhancement. For example, we could store the pinned state in a separate store which can be snapshotted.
On a separate note, we actually have a workaround for this bug in develop whereby we've lowered the priority for the affected notifications. We should revert the priority to PRIORITY.SETUP_CTA_LOW in this PR, we don't need to wait until it's merged.
|
Size Change: +672 B (+0.03%) Total Size: 2.16 MB ℹ️ View Unchanged
|
|
Build files for a2f9607 have been deleted. |
|
Thank you for the kind review, @techanvil! I've updated the PR to instead persist the pinned notifications in browser storage (following the pattern for "seen notifications"). I've also removed the custom priority for the multi-step notifications. This is now back with you for another look. Thanks! |
There was a problem hiding this comment.
Thanks @nfmohit! Nice work, this working well in my testing.
I've left a few super nitpicky comments, other than that this is looking great.
It would also be useful to include the Enhanced Measurement Banner flow in the QAB, please can you update it accordingly?
| const { pinNotification } = | ||
| registry.dispatch( CORE_NOTIFICATIONS ); | ||
|
|
||
| expect( () => pinNotification() ).toThrow( |
There was a problem hiding this comment.
Super trivial, the anonymous function isn't needed here:
| expect( () => pinNotification() ).toThrow( | |
| expect( pinNotification ).toThrow( |
| const { unpinNotification } = | ||
| registry.dispatch( CORE_NOTIFICATIONS ); | ||
|
|
||
| expect( () => unpinNotification() ).toThrow( |
There was a problem hiding this comment.
As above:
| expect( () => unpinNotification() ).toThrow( | |
| expect( unpinNotification ).toThrow( |
| const { getPinnedNotificationID } = | ||
| registry.select( CORE_NOTIFICATIONS ); | ||
|
|
||
| expect( () => getPinnedNotificationID() ).toThrow( |
There was a problem hiding this comment.
As above:
| expect( () => getPinnedNotificationID() ).toThrow( | |
| expect( getPinnedNotificationID ).toThrow( |
| const onOAuthNavigation = useCallback( async () => { | ||
| await pinNotification( id, NOTIFICATION_GROUPS.SETUP_CTAS ); | ||
| }, [ id, pinNotification ] ); |
There was a problem hiding this comment.
We can simply return the result of pinNotification() here, the returned Promise will be awaited by the caller:
| const onOAuthNavigation = useCallback( async () => { | |
| await pinNotification( id, NOTIFICATION_GROUPS.SETUP_CTAS ); | |
| }, [ id, pinNotification ] ); | |
| const onOAuthNavigation = useCallback( | |
| () => pinNotification( id, NOTIFICATION_GROUPS.SETUP_CTAS ), | |
| [ id, pinNotification ] | |
| ); |
|
Thank you for the review, @techanvil! I've addressed all the comments and have updated the QAB to include a test of the EM banner. Please let me know if this looks good now, thanks! |
Summary
Addresses issue:
Relevant technical choices
This PR adds the ability to pin notifications in case they need to appear after a subsequent page load, such as an OAuth flow for continuity.
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