[FIX] Notificiation: set a default notification store #4188
Conversation
Adapt the setup of the notification override following odoo/o-spreadsheet#4188 Task-3919166
rrahir
force-pushed
the
saas-17.2-min-integration-rar
branch
from
ef3f20c
to
8bcaeb7
Compare
rrahir
force-pushed
the
saas-17.2-min-integration-rar
branch
from
8bcaeb7
to
c83bc22
Compare
rrahir
force-pushed
the
saas-17.2-min-integration-rar
branch
9 times, most recently
from
2a26177
to
eecbebc
Compare
Adapt the setup of the notification override following odoo/o-spreadsheet#4188 Task-3919166
| **optional**: | ||
|
|
||
| - `notifyUser` | ||
| A function used to notify the user. It supports several levels of severity as well as a sticky behaviour. |
There was a problem hiding this comment.
seems strange to say the it supports "several levels of severity as well as a sticky behaviour" since the default one only does a window.alert with no nuanced behaviour.
There was a problem hiding this comment.
the API has to support it.
| notifyUser: (notification: InformationNotification) => any; | ||
| raiseError: (text: string, callback?: () => void) => any; | ||
| askConfirmation: (content: string, confirm: () => any, cancel?: () => any) => any; |
There was a problem hiding this comment.
Do we really want to remove these methods from the env ? Leaving them in the SpreadsheetChildEnv doesn't cost much, and makes interacting with them a bit less annoying.
There was a problem hiding this comment.
@VincentSchippefilt disagreed on that point and I don't have any strong opinion on this. The bad bard is that is had to become
notifyUser: (notification: InformationNotification) => this.notificationStore.notifyUser(notification);Everything we put in the env is set in stone as we pass direct references. If we want the notifications, or the model to act as actual props, we should have (nearly) every entry onf the env being dynamic references and not static or binded as we have today.
Following the introduction of stores, the minimal setup has evolved as it requires to instanciate a some stores before the creation of a spreadsheet app. The documentation was not updated accordingly. More specifically, the only store that we required to instantiate manually was the `NotificationStore` and we had to add it to the App env as well. These last step are a pain for developpers that only want to set a quick and minimal integration. This commit sets default notification methods (raiseError, notifyUser, askConfirmation) which can either be passed as props to a `Spreadsheet` component and/or be set by invoking the `NotificationStore`. Task: 3919166
rrahir
force-pushed
the
saas-17.2-min-integration-rar
branch
from
eecbebc
to
c0dfe75
Compare
Following the introduction of stores, the minimal setup has evolved as it requires to instanciate a some stores before the creation of a spreadsheet app. The documentation was not updated accordingly.
More specifically, the only store that we required to instantiate manually was the
NotificationStoreand we had to add it to the App env as well. These last step are a pain for developpers that only want to set a quick and minimal integration.This commit sets default notification methods (raiseError, notifyUser, askConfirmation) which can be updated by calling
NotificationStore.updateNotificationCallbacks.Task: 3919166
Description:
description of this task, what is implemented and why it is implemented that way.
Task: : 3919166
review checklist