-
Notifications
You must be signed in to change notification settings - Fork 137
Add in-system notifications for students #353
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 9.x
Are you sure you want to change the base?
Add in-system notifications for students #353
Conversation
SahiruWithanage
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Samindi!
Nice work on the frontend notifications feature! The implementation looks clean and integrates well with the header component. I have a few suggestions that would improve the code quality:
The console.log statements in toggleNotifications() and dismissNotification() methods should be removed before production deployment - they're helpful during development but can clutter the console in production.

Consider adding cleanup for the HTTP subscriptions by implementing ngOnDestroy() - this prevents potential memory leaks if the component is frequently mounted/unmounted.
These are relatively minor issues in an otherwise solid implementation. Great job on getting this feature built!
I will review the backend as well if I find any issue there that the front end needs to change I will update this comment.
JoeMacl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gave it a test and everything works smoothly! Notifications load, dismiss, and clear out just as expected. Nice work!
Just adding onto Sahiru’s suggestion, maybe consider adding a click-outside-to-close behavior for the panel.
Great job overall!
|
LGTM. |
SahiruWithanage
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested this on my local 9.x branch with the backend changes as well. Since this PR is frontend, I verified it alongside the backend for this feature, but it still didn’t work. Clicking the notification button doesn’t show the “no notifications” message or the cancel button, basically, nothing appears at all.
Description
Added a notification panel to display in-system notifications for students. This was created as part of the Staff Grant Extension feature as the design document indicated the need for displaying extension notifications to the student on the homepage.
As seen below, the bell icon is where students can access their notifications:
When they receive a notification, a red number will appear indicating how many notifcations they currently have:
When the student clicks on this notification, the following panel will appear:
They can click on the black "X" to delete a notification (this deletes it from the database) or they can choose to delete all, which deletes all the notifications for that student from the database.
Additionally, this PR depends on PR:69. The backend PR contains the table migration, model creation etc to make this feature work.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
To test this feature, you will need to login as the student (username: astudent, password:password) and ensure that the component is visible. You will most likely see the following message "No notifications" as your database will be empty.
You can, however, pull the backend code from PR:69 and test that the front end is populated when you create a notification in the backend for the user. Follow the instructions in the backend PR to create a notification if you wish to test the integration.
Testing Checklist:
Checklist: