feat(NotificationTray): Style the unread notifications#378
Conversation
AaronCoplan
left a comment
AaronCoplan
left a comment
There was a problem hiding this comment.
Couple small changes, but once you make those it'll be good to merge! If the type import isn't doable, no worries, just thought it might be worth looking into.
|
|
||
| type State = {| | ||
| unreadCount: number, | ||
| notificationsObjects: Array<{ |
There was a problem hiding this comment.
Notification.react exports its props, so could we potentially import this instead of redefining the type here? Not 100% sure if this is doable but maybe something like:
import type { Props as NotificationProps } from ...
Then you can type this as notificationsObjects: Array<NotificationProps>,
There was a problem hiding this comment.
Its doable, I would need to
a) import the Notification file directly, or
b) export the type from index.js
(I already tried this but as nothing else had this pattern I thought I would type a similar object). Do you have a preference?
There was a problem hiding this comment.
Let's export the type from index.js similar to lines 4-18 in that file (exporting event types).
| */ | ||
| +time?: string, | ||
| /** | ||
| * The time displayed within the Notification |
There was a problem hiding this comment.
Looks like an accidental typo, change to something about read/unread.
|
@AaronCoplan Back to you for review. I am not 100% confident I handled the type export/import the way you were thinking. If you could give it another quick lookover and provide some options that would be great.... And sorry for the copy/paste error... ooops :) |
|
@mattsoftware Looks good! The export works for me - tested by adding an additional prop that does not match the type in |
|
🎉 This PR is included in version 1.26.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
Adds some background styling when showing unread notifications in the notifications tray
See #376 for conversation around this pull request.