-
Notifications
You must be signed in to change notification settings - Fork 181
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
Fix: Revert notification changes #3763
Conversation
Mikehrn
commented
Jan 3, 2025
•
edited
Loading
edited
- Revert back to old notification system of only triggering one notification at the time
- Add option to hide toasts in compostables, and handle them in the parent in the case of server invites
}) | ||
|
||
isOpen.value = false | ||
} catch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change of any of them failing is slim, main cause probably network errors, as validation is already done, but this adds at least some basic error handling in the case this happens
|
||
/** | ||
* Persisting toast state between reqs and between CSR & SSR loads so that we can trigger | ||
* toasts anywhere and anytime | ||
*/ | ||
const useGlobalToastState = () => | ||
useSynchronizedCookie<Optional<ToastNotification[]>>('global-toast-state') | ||
useSynchronizedCookie<Optional<ToastNotification>>('global-toast-state') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm wondering what will happen when the page loads in for someone who already has this cookie set with an array of toast notifications, hopefully the code doesn't break (expects a single object, finds an array).
its probably fine, but we should probably validate this assumption locally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm