-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Feature: #2471 notification on_close #2519
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
base: develop
Are you sure you want to change the base?
Conversation
Hello there, I have some doubts that I would like to share. First and foremost this is my work until now:
The final result can be seen by the following code and prints:
My questionI don't understand if the changes requested are these or if the reasons"forced" and "timeout" should be something that the user never has to actually 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.
please do not commit file that are not related to the PR
I suppose this PR is missing a front-end part ?
How should that part be handled ?
main.py
Outdated
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.
this file shouldn't be committed
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.
this file shouldn't be committed
Hello there @FredLL-Avaiga, thank you for the reply
For now there are no front-end modifications, I just wanted to clarify my previous question so that I am aware on what needs to be done |
Hello there @FabienLelaquais, I don't understand if the changes requested are the ones that I made (the user inputs the reason whenever he closes the notification), or if the reasons "forced" and "timeout" should be something that the user never has to actually state. You also said this:
However, when I try create multiple notifications there is a limit of 5 notifications that are displayed simultaneously, and whenever there is a sixth notification, one of the older notifications will be removed (in a First In First Out manner) . Looking forward to hearing from you |
Hi @Andre-Pestana0 Now. Does that makes sense? |
@FabienLelaquais |
Hello there @FredLL-Avaiga @FabienLelaquais I think I'm almost at the solution, but I'm missing something...
I have been trying to understand how to to gather this data from the backend, but with no success. |
taipy/gui/gui.py
Outdated
): | ||
if notification_id and on_close: | ||
self._notification_callbacks[notification_id] = on_close |
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.
on_close
should be transformed into a string so that it can be managed by the front-end. (check the builder code for inspiration)
so that there is a dispatch from the front-end to the back-end only if on_close
is defined.
taipy/gui/gui.py
Outdated
@@ -2411,19 +2414,24 @@ def _notify( | |||
system_notification: t.Optional[bool] = None, | |||
duration: t.Optional[int] = None, | |||
notification_id: t.Optional[str] = None, | |||
on_close: t.Optional[t.Callable[[State, str, str], None]] = None, |
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.
on_close
can also be a string (name of a function)
(event: SyntheticEvent | null, reason: CloseReason, key?: SnackbarKey) => { | ||
const final_reason = reason === "timeout" ? "timeout" : "forced"; | ||
if (key) { | ||
dispatch(createDeleteNotificationAction(key.toString(), final_reason)); |
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.
you need to use another dispatch here: sendAction if on_close is defined
return { | ||
type: Types.DeleteNotification, | ||
snackbarId, | ||
reason, |
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.
no need
Hello @FredLL-Avaiga, Thank you so much for your reply. I will make sure to implement the requested changes. Sorry for bringing this up again, but I'm still having some difficulty understanding how the communication between the frontend and backend is happening. I understand that by using the dispatch function, I'll be sending information to the frontend, but I'm not quite sure how to inspect what data is being sent and how to retrieve that same data from the backend (but understanding how is it being sent will help me figure out this part). Could you please help me understand how I can view the data being sent from the frontend to the backend? I've tried using console.log, which helped a bit, but I'm still missing some details. I appreciate your help and hope I’m not burdening you with too many questions. Thank you again! |
In this case, you want to trigger a back-end callback from the front-end. You can find an example of its use in the Button component |
Hello there @FredLL-Avaiga, I've been working on the notification callback for on_close and this is what I’ve done so far: Backend:
Frontend:
At one point, I was able to make this work, but after making some changes, the callback stopped being passed correctly. The on_close value in the frontend is always undefined. I suspect the issue might be happening during the transition from the backend to the frontend. I’ve checked the backend and confirmed that the on_close_str is not None before sending the notification. Would appreciate any pointers on what might be causing this or what I might have missed. Thank you so much for your time! |
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.
you need to change the createNotificationAction to handle the on_close callback name
.gitignore
Outdated
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.
we want the package-lock.json to be committed when it is changed willingly
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 guess you didn't want to commit this ?
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.
Yes, I tried to add it to the gitignore for now so it wouldn't commit that
taipy/gui/gui.py
Outdated
@@ -369,6 +369,8 @@ def __init__( | |||
The returned HTML content can therefore use both the variables stored in the *state* | |||
and the parameters provided in the call to `get_user_content_url()^`. | |||
""" | |||
# Notification callbacks | |||
self._notification_callbacks = {} |
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.
please remove this
the callback name should be sent to the front-end and come back from the front-end
console.log(callback) | ||
if (key) { | ||
if (true) { //Should be if(callback) but callback is always undefined | ||
createSendActionNameAction(notification?.notificationId, module, "on_notification_closed", final_reason); |
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.
should be dispatch(createSendActionNameAction(...))
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.
Thank you very much
const notificationClosed = useCallback( | ||
(event: SyntheticEvent | null, reason: CloseReason, key?: SnackbarKey, callback?: string) => { | ||
const final_reason = reason === "timeout" ? "timeout" : "forced"; | ||
console.log(callback) |
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.
do not commit console calls
@@ -864,13 +895,21 @@ export const createNotificationAction = (notification: NotificationMessage): Tai | |||
snackbarId: notification.snackbarId | |||
}); | |||
|
|||
export const createDeleteNotificationAction = (snackbarId: string): TaipyDeleteNotificationAction => { | |||
export const createDeleteNotificationAction = (snackbarId: string, callback?: string): TaipyDeleteNotificationAction => { |
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.
no need for callback here
Hello there. I believe I did almost all of the requested changes. My only problem now is regarding the callback that is always undefined Notification.tsx
gui.py
In the gui.py i have done some prints and I am positive that "on_close_str" is indeed a valid string. I have tried to use Is there anything else I'm missing? |
taipy/gui/gui.py
Outdated
) | ||
return notification_id | ||
|
||
def _close_notification( | ||
self, | ||
notification_id: str, | ||
reason: t.Optional[str] = "timeout", |
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.
default value should not be timeout
but user_action
taipy/gui/gui.py
Outdated
if notification_id in self._notification_callbacks: | ||
callback = self._notification_callbacks.pop(notification_id) | ||
callback(self, notification_id, reason) | ||
|
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.
remove this
export const createSendAction = (name: string): TaipySendAction => { | ||
return { | ||
type: Types.Action, | ||
name, | ||
}; | ||
}; | ||
|
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.
use createSendActionNameAction
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.
and add optionals reason
and on close
to NotificationMessage
taipy/gui/gui.py
Outdated
@@ -1458,14 +1458,15 @@ def __send_ws_download(self, content: str, name: str, on_action: str, module: st | |||
) | |||
|
|||
def __send_ws_notification( | |||
self, type: str, message: str, system_notification: bool, duration: int, notification_id: t.Optional[str] = None | |||
self, type: str, message: str, system_notification: bool, duration: int, notification_id: t.Optional[str] = None, reason: t.Optional[str] = None |
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.
need to add on_close_str here
) -> None: | ||
payload = { | ||
"type": _WsType.ALERT.value, | ||
"nType": type, | ||
"message": message, | ||
"system": system_notification, | ||
"duration": duration, | ||
"reason": reason, |
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.
and there too (on_close_str)
Hello there, thank you for the reply.
Im trying to call I added a Thank you so much for your time. |
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 suppose you still need to change the case Types.SetNotification ...
Hello there, Thank you so much for your help, i had to do that and
Now everything is working! Thank you so much for your time |
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.
Almost there
Well done
if (callback) { | ||
dispatch(createSendActionNameAction(notification?.notificationId, module, callback, final_reason)); | ||
} | ||
dispatch(createDeleteNotificationAction(key.toString())); |
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 don't think this is needed here
@@ -114,6 +116,7 @@ interface TaipyNotificationAction extends TaipyBaseAction, NotificationMessage { | |||
|
|||
interface TaipyDeleteNotificationAction extends TaipyBaseAction { | |||
snackbarId: string; | |||
callback?: 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.
not needed
@@ -861,7 +865,8 @@ export const createNotificationAction = (notification: NotificationMessage): Tai | |||
system: notification.system, | |||
duration: notification.duration, | |||
notificationId: notification.notificationId, | |||
snackbarId: notification.snackbarId | |||
snackbarId: notification.snackbarId, | |||
on_close: notification?.on_close, |
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.
in ts files, we use camelCase and not snake_case
taipy/gui/gui.py
Outdated
else: | ||
_warn(f"Notification on_close callback '{on_close}' is not a valid function.") | ||
|
||
elif callable(on_close): |
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.
use _is_function from utils
_warn(f"Notification on_close callback '{on_close}' is not a valid function.") | ||
|
||
elif callable(on_close): | ||
on_close_str = on_close.__name__ |
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.
lambda functions are not handled
Hello there @FredLL-Avaiga |
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.
any test ?
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.
do not commit this file
.gitignore
Outdated
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.
do not commit this file
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 will not include in the final version
}; | ||
const notificationClosed = useCallback( | ||
(event: SyntheticEvent | null, reason: CloseReason, key?: SnackbarKey, callback?: string) => { | ||
const final_reason = reason === "timeout" ? "timeout" : "forced"; |
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.
this could be integrated in the dispatch
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.
Integrated
const notificationClosed = useCallback( | ||
(event: SyntheticEvent | null, reason: CloseReason, key?: SnackbarKey, callback?: string) => { | ||
const final_reason = reason === "timeout" ? "timeout" : "forced"; | ||
if (key && callback) { |
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.
Why test key ?
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.
It was removed
taipy/gui/gui_actions.py
Outdated
@@ -122,7 +124,8 @@ def close_notification(state: State, id: str) -> None: | |||
""" | |||
if state and isinstance(state._gui, Gui): | |||
# Send the close command with the notification_id | |||
state._gui._close_notification(id) # type: ignore[attr-defined] |
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.
clean up please
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.
Done
6e728cb
to
5e24ea1
Compare
Hello there @FredLL-Avaiga |
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.
No need to test the action creation, but we need to test
1/ on the backend, on_close
and reason
are present in the payload when set
2/ on the frontend, a callback action is created when onClose is specified and a notification is closed (by system or user)
You also need to change your PR to remove the files that shouldn't be modified (.gitignore, package-lock.json, main.py)
mockDispatch(action); | ||
}); | ||
|
||
expect(mockDispatch).toHaveBeenCalledWith( |
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.
you're only testing the createSendActionNAmeAction
here ?
What type of PR is this? (check all applicable)
Description
Implementation of a way to now if a notification was closed or not, including a callback function for the notification and the reason behind it.
Related Tickets & Documents
How to reproduce the issue
Please replace this line with instructions on how to reproduce the issue or test the feature.
Other branches or releases that this needs to be backported
Describe which projects this change will impact and that needs to be backported.
Checklist
We encourage you to keep the code coverage percentage at 80% and above.