refactor: cleanup notifications system #3926
Conversation
|
CLA Assistant Lite All Contributors have signed the CLA. |
| const notificationsReducer = handleActions<NotificationsState, Notification | KeyPayload>( | ||
| { | ||
| [NOTIFICATION_ACTIONS.SHOW]: (state, { payload }: Action<Notification>) => { | ||
| // getNotificationsFromTxType can return `null` notifications | ||
| return payload ? [...state, payload] : state | ||
| }, | ||
| [NOTIFICATION_ACTIONS.CLOSE]: (state, { payload }: Action<KeyPayload>) => { | ||
| return state.map((notification) => { | ||
| return notification.options?.key === payload.key ? { ...notification, dismissed: true } : notification | ||
| }) | ||
| }, | ||
| [NOTIFICATION_ACTIONS.CLOSE_ALL]: (state) => { | ||
| return state.map((notification) => ({ ...notification, dismissed: true })) | ||
| }, | ||
| [NOTIFICATION_ACTIONS.DELETE]: (state, { payload }: Action<KeyPayload>) => { | ||
| return state.filter((notification) => notification.options?.key !== payload.key) | ||
| }, | ||
| [NOTIFICATION_ACTIONS.DELETE_ALL]: () => { | ||
| return [] | ||
| }, | ||
| }, | ||
| [], | ||
| ) | ||
|
|
||
| export const showNotification = ( | ||
| payload: Pick<Notification, 'message' | 'options'>, | ||
| ): ThunkAction<SnackbarKey, AppReduxState, undefined, AnyAction> => { | ||
| return (dispatch): SnackbarKey => { | ||
| const action = createAction<Notification>(NOTIFICATION_ACTIONS.SHOW) | ||
|
|
||
| const key = payload.options?.key || Math.random().toString(32).slice(2) | ||
|
|
||
| dispatch( | ||
| action({ | ||
| ...payload, | ||
| options: { ...payload.options, key }, | ||
| }), | ||
| ) | ||
|
|
||
| return key | ||
| } | ||
| } | ||
| export const closeNotification = createAction<KeyPayload>(NOTIFICATION_ACTIONS.CLOSE) | ||
| export const closeAllNotifications = createAction(NOTIFICATION_ACTIONS.CLOSE_ALL) | ||
| export const deleteNotification = createAction<KeyPayload>(NOTIFICATION_ACTIONS.DELETE) | ||
| export const deleteAllNotifications = createAction(NOTIFICATION_ACTIONS.DELETE_ALL) | ||
|
|
||
| export const selectNotifications = (state: AppReduxState): NotificationsState => { | ||
| return state[NOTIFICATIONS_REDUCER_ID] | ||
| } |
There was a problem hiding this comment.
After working on web-core, it really doesn't make sense to separate these into separate files in my opinion.
ESLint Summary View Full Report
Report generated by eslint-plus-action |
|
Good refactoring, but it broke some of the notifications. I tested only the address book ones.
|
|
@katspaugh, seems like both were CSS selector issues and these should now be addressed. |
|
Checked all the notifications I could find and they look fine. Regarding
I tried to use safes that are 1.0.0 or 1.1.1 and I didn't get any notification, not when accessing it nor when I upgrade them. So I'm not sure how to check this one |
I've adjusted the action that shows the update notification. If you: a) have an older Safe open and connect to an owner the notification should be displayed. Clicking on it should take you to the details settings where you can update the Safe. |
|
Ok, the notification is there. Looks good to me |
What it solves
An introduction to safe-global/safe-pm#83
How this PR fixes it
The current notification system has been refactored:
showNotification,close/closeAllNotification(s)anddelete/deleteAllNotification(s)and the syntax follows Notistack as closely as possible.notificationsstore.None of the above adjustments should yet reflect/alter the UI in it's current state.
How to test it
Interact with the Safe and observe notifications functioning as before.
One notable notification to check, which includes an
onClickhandler, is dispatched when the Safe requires an update. I removed some unnecessary code that is responsible for address book interaction notifications. They should still work as before as well.