[PAY-176] Implement NotificationPanel redesign#1308
Conversation
|
Preview this change https://demo.audius.co/dj-pay-176-finalize-desktop-notification-redesign |
|
Preview this change https://demo.audius.co/dj-pay-176-finalize-desktop-notification-redesign |
| ) | ||
|
|
||
| // Based on how notification types are defined, we need to cast like this. | ||
| // In the future we should select user/users/entity/entities in each notif. |
There was a problem hiding this comment.
+1. I think ideally the NotificationPanel can use some mapping of NotificationType => Notification element and avoid the wrapper in the future
There was a problem hiding this comment.
yeah the mapping def makes sense! im not sure if the wrapping is smart enough to figure out that if the type is x, then the component is y, and the prop shape is z, though, if that makes sense?
There was a problem hiding this comment.
I'm thinking in the future these notifs will all have the same props structure so we just pass it in the same to each one, but maybe not?
There was a problem hiding this comment.
they will be diff in the sense that they have BaseNotif, but then other things like value vs reaction vs whatevs i think!
There was a problem hiding this comment.
Hmm I'm thinking like:
Somewhere:
notificationTypeMap = {
[NotificationType.Follow]: FollowNotification,
//... etc
}NotificationPanel.tsx:
// ....
notifications.map(notification =>
<notificationTypeMap[notification.type] key={notification.id} {...notification} />Is that what you're thinking?
There was a problem hiding this comment.
yeah the main issue with that with ts though is that Notification is a discriminated union, so we need to get the component from the type, and also merge in the notification with the corresponding type, that's why the switch case works here cause it aligns the notification type so the component + notification type checks correctly, not sure if im being clear!
There was a problem hiding this comment.
Makes sense, gotta appease the typescript engine. Also makes it easier to reason about anyway probably. I'm ok with switch cases, the only slight concern for me is the intermediate React component. I feel like I remember that causing wrapping <divs> and/or having perf implications, but it's been a while and I'm probably wrong and it's not an issue anymore or never was...
There was a problem hiding this comment.
yeah i def hear you on the extra component... will be thinking of a way to make typescript happy while also avoiding a wrapper
| const handleClick: MouseEventHandler = useCallback( | ||
| event => { | ||
| onClick?.(event) | ||
| if (!disableClosePanel) { |
There was a problem hiding this comment.
This works, but after thinking about it, what if we don't close the NotificationsPanel on clicks, but rather on navigations? The goal is to close it whenever the user navigates away, right? I guess we'd need both as we probably want to close it on Twitter modal popups? I'm a bit uninformed here for sure.
There was a problem hiding this comment.
yeah this is good thinking for sure! yeah do we want to close in on twitter? we don't currently which i think makes sense. so def agree with the idea that it should be on navigates, how would we do that for the complex interaction of click follow notif with multiple follows -> opens user list modal -> click on a user goes to profile and closes notification? just have an event listener for url changes in panel?
There was a problem hiding this comment.
Ah if we don't currently then no - I'm a little hazy on the ins/outs of when we close it currently sorry! All the route changes should be going through the store, right? Maybe a saga listens for route pushes and put an action to hide notif panel? Seems like a misuse of sagas, but maybe it's fine 😁 Could also be a reducer but i think we slice those up so not sure a notif panel reducer would see the route actions
There was a problem hiding this comment.
no worries youre thinking is spot on. ill think about things more and see if i come to a sensible way of handling this!
rickyrombo
left a comment
There was a problem hiding this comment.
One more thing - did we check electron? Saw the panel had some isElectron flag
that is a good call, i noticed the isElectron was never used in the original code, so looked safe to remove |
This reverts commit 8430e6f.
This reverts commit 8430e6f.
Description
Implements new NotificationPanel with new redesigned notifications