-
Notifications
You must be signed in to change notification settings - Fork 4
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
New hook to subscribe to announcements #405
base: main
Are you sure you want to change the base?
Conversation
…ements). Signed-off-by: Florent MILLOT <[email protected]>
@@ -98,6 +99,7 @@ | |||
"utf-8-validate": "^5.0.2", | |||
"@react-hook/window-size": "^3.1.1", | |||
"react-resizable": "^3.0.4", | |||
"reconnecting-websocket": "^4.4.0", |
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.
double declaration, can be removed
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.
Actually we need it in both peer and dev deps no ?
export function useAnnouncementsSubscriber( | ||
webSocketUrl: string, | ||
token: string | ||
) { |
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.
To keep concerns separate, shouldn't we pass the token in the URL directly in the hook, and then the hook does not mind about token issues ? Either it could connect or not.
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.
If I do that I'll have to read the string to know if the token is present or not, seems worse to me.
How would you do that ?
Add useAnnouncementsSubscriber hook to subscribe via websocket to global notifications (announcements).
When new message, it display a snackbar at the top of the page.
from gridsuite/gridstudy-app#1902 and gridsuite/gridexplore-app#346