Remove use of actionCall, fix updateViewed, manage state with objects…#375
Conversation
… instead of arrays, simplify types
Gudahtt
left a comment
There was a problem hiding this comment.
Unit tests appear to be failing.
Also, the config and the state of this controller seem redundant 🤔 I noticed that the state is changing as notifications are seen, but the config remains unchanged (with its original isShown values intact).
Maybe it would make more sense for the notification metadata to be in the config, and just the isShown value stored in state? Or if it's more convenient to have all of the metadata in state, we can leave the redundancy there for now (until the migration to the new base controller), and just eliminate isShown from the notification config (or eliminate the config completely? 🤷 ).
| */ | ||
| export interface NotificationConfig extends BaseConfig{ | ||
| allNotifications: Notification[]; | ||
| allNotifications: NotificationMap | undefined; |
There was a problem hiding this comment.
Why | undefined? I wouldn't expect this controller to be constructed in the first place if there are no notifications
There was a problem hiding this comment.
Removed in 0081fa4 (This was left over from when I was anticipating a possible need to only add notifications at some point after the controller had been constructed, via another public method. But it turns out we won't be doing that.)
371db50 to
9bb13f5
Compare
#375) * Remove use of actionCall, fix updateViewed, manage state with objects instead of arrays, simplify types * Only store isShown in state, not in the config * Remove undefined possibility from the allNotifications type * Fix NotificationController tests * Fix logic in NotificationController _addNotifications: ensure current state notifications are not deleted.
#375) * Remove use of actionCall, fix updateViewed, manage state with objects instead of arrays, simplify types * Only store isShown in state, not in the config * Remove undefined possibility from the allNotifications type * Fix NotificationController tests * Fix logic in NotificationController _addNotifications: ensure current state notifications are not deleted.
#375) * Remove use of actionCall, fix updateViewed, manage state with objects instead of arrays, simplify types * Only store isShown in state, not in the config * Remove undefined possibility from the allNotifications type * Fix NotificationController tests * Fix logic in NotificationController _addNotifications: ensure current state notifications are not deleted.
#375) * Remove use of actionCall, fix updateViewed, manage state with objects instead of arrays, simplify types * Only store isShown in state, not in the config * Remove undefined possibility from the allNotifications type * Fix NotificationController tests * Fix logic in NotificationController _addNotifications: ensure current state notifications are not deleted.
#375) * Remove use of actionCall, fix updateViewed, manage state with objects instead of arrays, simplify types * Only store isShown in state, not in the config * Remove undefined possibility from the allNotifications type * Fix NotificationController tests * Fix logic in NotificationController _addNotifications: ensure current state notifications are not deleted.
Compares to #329
This PR makes some changes to the NotificationsController added with #329
The following changes are made:
actionCallmethod, and therefore theactionFunctionproperty are removed. These will be handled on the client side.actioninterface can be removed and theNotificationinterface can be simplifiedupdateViewedmethod