(feat) Notification bell doesn't show notifications#993
Conversation
|
@coderatomy Thanks for this. The current design does not match exactly the suggested UX in #992 (notification circle should be half outside the main bell circle). I also think we should experiment with increasing the font size of the notification count. In addition, have you tested the behavior when all notifications are read already? Does the icon work as expected (it should not show 0, but should just disappear). |
|
Everything works as expected. For the design @mehreeee updated it to this |
NdibeRaymond
left a comment
NdibeRaymond
left a comment
There was a problem hiding this comment.
test the functionality and it works @tuxology. Just a few comments and we can merge this @coderatomy . Nice work!
| ref={buttonRef} | ||
| className={clsx(classes.notification, commonClasses.iconBox)} | ||
| style={{ cursor: 'pointer' }} | ||
| style={{ position: 'relative', cursor: 'pointer' }} |
There was a problem hiding this comment.
can you help move this to notificationButtonStyles.js?
| </span> | ||
| ) | ||
| } | ||
| <Notifications style={{ color: colors.primary, fontSize: 24 }} /> |
There was a problem hiding this comment.
this too. move to notification style file
|
Something we should probably do in the future is to implement push notification. Right now users don't get notified unless they perform some kind of action that results in the frontend hitting the notification endpoint |
|
@coderatomy Ok, I see that there was an update to the design 🤔 Its fine, we go with this design then. Note that the icon+notification bubble together is centered and has consistent padding along the sides so it looks balanced. In your implementation we see the notification bubble touching the outer circle edge. In addition, lets do changes requested by @NdibeRaymond as well Thanks @NdibeRaymond for testing. We'll merge this soon. |
NdibeRaymond
added
create-test-vm
4 participants
Summary
Implements notifiers in case of numbers.
Closes #992
Changes
Screenshots